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

[Robert S Peterson <rpeterso@redhat.com>]

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.  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.