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:
>
> Use normal write file operations rather than AOPS prepare_write and
> commit_write when the backing filesystem requires special locking.
> 
> ..
>
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -44,6 +44,11 @@
>   * backing filesystem.
>   * Anton Altaparmakov, 16 Feb 2005
>   *
> + * 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.

>   * Still To Fix:
>   * - Advisory locking is ignored here.
>   * - Should use an own CAP_* category instead of CAP_SYS_ADMIN
> @@ -74,6 +79,7 @@
>  #include <linux/completion.h>
>  #include <linux/highmem.h>
>  #include <linux/gfp.h>
> +#include <linux/mount.h>
> 
>  #include <asm/uaccess.h>
> 
> @@ -791,7 +797,8 @@ static int loop_set_fd(struct loop_devic
>                  */
>                 if (!file->f_op->sendfile)
>                         goto out_putf;
> -               if (aops->prepare_write && aops->commit_write)
> +               if (!(file->f_vfsmnt->mnt_sb->s_type->fs_flags & FS_AOPS_NEED_LOCKING) &&
> +                       aops->prepare_write && aops->commit_write)
>                         lo_flags |= LO_FLAGS_USE_AOPS;
>                 if (!(lo_flags & LO_FLAGS_USE_AOPS) && !file->f_op->write)
>                         lo_flags |= LO_FLAGS_READ_ONLY;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 9d96749..3def72e 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -88,6 +88,7 @@ extern int dir_notify_enable;
>  /* public flags for file_system_type */
>  #define FS_REQUIRES_DEV 1
>  #define FS_BINARY_MOUNTDATA 2
> +#define FS_AOPS_NEED_LOCKING 4 /* Filesystem aops have special locking needs */
>  #define FS_REVAL_DOT   16384   /* Check the paths ".", ".." for staleness */
>  #define FS_ODD_RENAME  32768   /* Temporary stuff; will go away as soon
>                                   * as nfs_rename() will be cleaned up

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.

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.

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.

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.


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.