Evgeny Kotkov <[email protected]> writes: > >> - *read_only = FALSE; > >> + *read_only = (access(file_info->fname, W_OK) != 0); > > There are a few problems, because this approach adds a I/O syscall (that > checks access to a file by its name) into a fairly low-level function. > > Previously this function was analyzing the file information from the passed-in > apr_finfo_t structure. The new version, however, performs an I/O call, and > that leads to a couple of problems: > > 1) Potential performance regression, not limited to just the revert. > For example, all calls to open_pack_or_rev_file() in libsvn_fs_fs would > now start to make additional access() calls, thus slowing down the > repository operations. > > 2) This adds an opportunity for a desynchronization/race between the > information in apr_finfo_t and the result of access(), because access() > works by a filename. For example, in a case where a file is recreated > between the two calls, its result will correspond to a completely > different file, compared to the one that produced the apr_finfo_t. > > Overall, I have a feeling that solving this specific edge-case might not be > worth introducing these regressions (which look significant, at least to me).
While reviewing API changes on trunk, I remembered about this topic. We seem to have a regression with problems outlined as (1) and (2), which probably outweigh the benefit of fixing the specific edge-case. >From an RM standpoint, I think we should unblock this for the 1.15 release. I suggest we revert this change to clear the release path. If necessary, we can return to it after 1.15 is out of the door. Thoughts? Thanks, Evgeny Kotkov
