On Mon, 2006-03-27 at 16:44 -0800, Andrew Morton wrote: > > + * Extension of Anton's idea: Use normal write file operations rather than > > + * prepare_write and commit_write when the backing filesystem requires > > + * special locking. > > + * Robert Peterson <rpeterso@xxxxxxxxxx>, 01 Mar 2006 > > + * > The preferred convention is not to put changelogs into .c files. The > revision control system is where such info is kept. This is not a changelog. The changelog was above, as crafted by my git format-patch. I was merely following the convention set forth in the code by Anton Altaparmakov, who added similar comments to the code in a previous fix. Ten years from now, in the year 2016, do you think it's more likely that a kernel hacker trying to figure out the purpose of this fix will look at comments in the code or search through ten-year-old changelogs? > FS_AOPS_NEED_LOCKING is too poorly defined. "locking" of what?? All that > should be defined with some precision and documented at least in the > changelog and preferably in a code comment above the FS_AOPS_NEED_LOCKING > definition site. And the name "FS_AOPS_NEED_LOCKING" itself is very vague. I chose this constant as an alternative to my original because it was suggested by Alton A. If this was a concern, perhaps it should have been brought up when I submitted the patch the first or second time. > Plus we have no in-kernel users of this new flag from which to glean some > understanding of what it means, so the documentation requirements become > higher. Perhaps my changelog was too vague. I was under the impression that changelogs should be concise, but I'm willing to add as much verbage as necessary. I'll resubmit with my previous explanation of why I think the change is good (see below). > I don't think the fact that the filesystem does or doesn't use locking is > relevant to this patch. Why not call the thing FS_LOOP_USE_READ_WRITE? > AFter all, that's what it does. In my opinion, yes it is relevant. What's at issue here is not whether loop.c uses write vs. prepare_write/commit_write, but whether ANY driver should choose one over the other. Loop.c is just one known broken case. Anton's suggested constant FS_AOPS_NEED_LOCKING expresses that any interaction with the underlying fs from ANY source should take the underlying fs's special locking requirements into account and therefore should favor "write" to "prepare_write". That makes it more useful for future kernel growth and expansion, not just a one-shot kludge for loopback. Do you like FS_AVOID_PREPARE_WRITE better? I'm open to suggestions. > I assume this new flag is needed for some out-of-tree filesystem? If so, > the changelog should have described which one, and why it needs this flag, > and how it will be using it, etc. The change is immediately applicable to Red Hat's GFS which is out-of-tree. However, GFS2 will hopefully be in-tree soon. Plus, this change will likely apply to other clustered filesystems that require special locking. I don't have the ability to test cxfs and such, but I would guess that other clustered filesystems have the same issues with loopback circumventing proper cluster locking. > I'm not averse to putting some tweaks into core kernel to support > out-of-tree GPL code - if it's of significant benefit to the owners of that > code (like: our code will now run when loaded into unmodified vendor > kernels) and has a minor impact on the kernel.org tree, then why not? But > it does need to be a good change, and one which is carefully and completely > described, please. I did this earlier when I first submitted the patch on 01 March. And I quote: > This is an extension of Anton Altaparmakov's previous fix which allows > loop.c to use the aop->write rather than prepare_write/commit_write if > prepare_write/commit_write aren't available. > > Right now, the current loop.c uses aop->prepare_write/commit_write > unless there is no other option. However, due to special locking > requirements, some backing filesystems may prefer the use of aop->write > rather than prepare_write/commit_write. Since loop.c does not have > advisory locking, the backing fs should have a choice of which to use. > > In the case of GFS, for example, loop.c's use of aop->prepare_write > circumvents proper cluster locking and transaction building, so using > aop->write is the right thing for loop.c to do. > > How the patch works: > If the backing filesystem has special locking requirements (new flag in > fs_flags) loop.c uses aop->write rather than prepare_write/commit_write. - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
