Robert S Peterson <rpeterso@xxxxxxxxxx> wrote: > > 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. And it was very poor. > 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? Ten years from now, if we put a not-a-changelog intot he source along with every patch, what would the source look like? > > 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. These things happen. > > 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 think I prefer FS_LOOP_USE_READ_WRITE. If we later find that that this exact flag can be reused elsewhere, we can look at reneming it then, based upon the new usage plus the old one. > > 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. OK. > > 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. Ah. It would have been best to reatain that in the changelog of the upissued patch. - 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
