Re: [PATCH] loop.c to use write ops for fs requiring special locking

[Andrew Morton <akpm@osdl.org>]

Robert S Peterson <rpeterso@redhat.com> 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@redhat.com>, 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.