[patch] loop.c to use write ops for fs requiring special locking

[patch] loop.c to use write ops for fs requiring special locking

[Robert S Peterson]

Hi All,

Below is a small patch to loop.c I'd like to see in the kernel.

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.

Feedback?

Regards,

Bob Peterson
rpeterso@redhat.com

--- linux-2.6.15.4.orig/drivers/block/loop.c    2006-02-10
01:22:48.000000000 -0600
+++ linux-2.6.15.4.patched/drivers/block/loop.c 2006-03-01
09:38:48.000000000 -0600
@@ -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
+ *
  * 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>

@@ -781,7 +787,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_REQUIRES_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 -pur linux-2.6.15.4.orig/include/linux/fs.h
linux-2.6.15.4.patched/include/linux/fs.h
--- linux-2.6.15.4.orig/include/linux/fs.h      2006-02-10
01:22:48.000000000 -0600
+++ linux-2.6.15.4.patched/include/linux/fs.h   2006-02-28
17:18:48.000000000 -0600
@@ -83,6 +83,7 @@ extern int dir_notify_enable;
 /* public flags for file_system_type */
 #define FS_REQUIRES_DEV 1
 #define FS_BINARY_MOUNTDATA 2
+#define FS_REQUIRES_LOCKING 4   /* Filesystem requires locking */
 #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

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

[Andrew Morton]

Robert S Peterson <rpeterso@redhat.com> wrote:
>
> Hi All,
> 
> Below is a small patch to loop.c I'd like to see in the kernel.
> 
> 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.

There's a reason why we must use prepare_write/commit_write rather than
->write() and ow-ow-my-brain-hurts I can't remember what it was.  Anton, do
you recall?

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

I think you'll find that cryptoloop doesn't work (see "ow-ow", above).

> --- linux-2.6.15.4.orig/drivers/block/loop.c    2006-02-10
> 01:22:48.000000000 -0600
> +++ linux-2.6.15.4.patched/drivers/block/loop.c 2006-03-01
> 09:38:48.000000000 -0600

(The patch is wordwrapped)

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

[Anton Altaparmakov]

On Wed, 1 Mar 2006, Andrew Morton wrote:
> Robert S Peterson <rpeterso@redhat.com> wrote:
> > Below is a small patch to loop.c I'd like to see in the kernel.
> > 
> > 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.
> 
> There's a reason why we must use prepare_write/commit_write rather than
> ->write() and ow-ow-my-brain-hurts I can't remember what it was.  Anton, do
> you recall?

It is simply that it is faster on all file systems that can use 
prepare_write/commit_write and the "performance hungry" would have 
complained if it were to be removed.

> > 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.
> > 
> 
> I think you'll find that cryptoloop doesn't work (see "ow-ow", above).

*blink* Why should it not work?

cryptoloop is exactly the reason for the performance hit with using 
->write rather than prepare_write/commit_write as we need to allocate a 
temporary page, do the transformation from the source page to the 
temporary page, then send the temporary page to ->write, and finally 
release the page.  Ugly but the only way to allow transformations 
in-flight I could see when writing the ->write using code for loop.c...

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

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

[Robert S Peterson]

On Thu, 2006-03-02 at 10:16 +0000, Anton Altaparmakov wrote:
> It is simply that it is faster on all file systems that can use 
> prepare_write/commit_write and the "performance hungry" would have 
> complained if it were to be removed.
(snip)
> 	Anton

Having heard no objections to my loop.c patch, when can I expect it to
be integrated into the 2.6 kernel?

Regards,

Bob Peterson

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

[Andrew Morton]

Robert S Peterson <rpeterso@redhat.com> wrote:
>
> On Thu, 2006-03-02 at 10:16 +0000, Anton Altaparmakov wrote:
> > It is simply that it is faster on all file systems that can use 
> > prepare_write/commit_write and the "performance hungry" would have 
> > complained if it were to be removed.
> (snip)
> > 	Anton
> 
> Having heard no objections to my loop.c patch, when can I expect it to
> be integrated into the 2.6 kernel?
> 

When we've remembered what Al's statement meant in
http://marc.theaimsgroup.com/?t=102129995600002&r=1&w=2

I knew at the time, but I've forgotten.  Maybe the issue went away..

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

[Anton Altaparmakov]

On Fri, 10 Mar 2006, Andrew Morton wrote:
> Robert S Peterson <rpeterso@redhat.com> wrote:
> > On Thu, 2006-03-02 at 10:16 +0000, Anton Altaparmakov wrote:
> > > It is simply that it is faster on all file systems that can use 
> > > prepare_write/commit_write and the "performance hungry" would have 
> > > complained if it were to be removed.
> > (snip)
> > > 	Anton
> > 
> > Having heard no objections to my loop.c patch, when can I expect it to
> > be integrated into the 2.6 kernel?
> 
> When we've remembered what Al's statement meant in
> http://marc.theaimsgroup.com/?t=102129995600002&r=1&w=2
> 
> I knew at the time, but I've forgotten.  Maybe the issue went away..

The comment is that the patch put forward by Urban in the above thread 
replaced the use of readpage for reading with file ->read and 
prepare_write/commit_write for writing with file ->write without thinking 
about it.  So he left out the entire transformation on the data which is 
what Al correctly complained about.

I.e. Urban's patch made loop only work for a "straight" loopback mount 
without any changes to the data (i.e. crypto) applied to it.

This is no longer relevant because my patch which is in current 2.6 
kernels (can't remember when I wrote it/when it got put into 2.6) did the 
conversion from prepare_write/commit_write to file ->write correctly so 
the data transformation still happens so that crypto still works with the 
loop driver.

To get back to Robert's patch that he is requesting to be included.  I 
think it is fine but the flag name could perhaps be better.  Perhaps 
"FS_AOPS_PRIVATE" or "FS_AOPS_SPECIAL" or "FS_AOPS_NEED_LOCKING" or 
even "FS_AOPS_REQUIRE_LOCKING" or something.  "FS_REQUIRES_LOCKING" just 
does not mean much and certainly would not suggest to me that no-one 
outside the file system should use the address space operations of the 
file system...  But maybe I am just bein picky.  (-:

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

[patch 2.6.16] loop.c to use write ops for fs requiring special locking [try #2]

[Robert S Peterson]

On Sat, 2006-03-11 at 00:36 +0000, Anton Altaparmakov wrote:
> On Fri, 10 Mar 2006, Andrew Morton wrote:
> > When we've remembered what Al's statement meant in
> > http://marc.theaimsgroup.com/?t=102129995600002&r=1&w=2
> This is no longer relevant because my patch which is in current 2.6 
> kernels (can't remember when I wrote it/when it got put into 2.6) did the 
> conversion from prepare_write/commit_write to file ->write correctly so 
> the data transformation still happens so that crypto still works with the 
> loop driver.
> 
> To get back to Robert's patch that he is requesting to be included.  I 
> think it is fine but the flag name could perhaps be better.  Perhaps 
> "FS_AOPS_PRIVATE" or "FS_AOPS_SPECIAL" or "FS_AOPS_NEED_LOCKING" or 
> even "FS_AOPS_REQUIRE_LOCKING" or something.  "FS_REQUIRES_LOCKING" just 
> does not mean much and certainly would not suggest to me that no-one 
> outside the file system should use the address space operations of the 
> file system...  But maybe I am just bein picky.  (-:

Here is a resubmission of my patch to loop.c, this time against the
2.6.16 kernel.

Andrew: Sounds like Anton answered your concerns.
Anton:  As per your suggestion, I changed the constant to your suggested
FS_AOPS_NEED_LOCKING.

Regards,

Bob Peterson
rpeterso@redhat.com

diff -pur linux-2.6.16/drivers/block/loop.c linux-2.6.16.patched/drivers/block/loop.c
--- linux-2.6.16/drivers/block/loop.c   2006-03-19 23:53:29.000000000 -0600
+++ linux-2.6.16.patched/drivers/block/loop.c   2006-03-24 10:49:10.000000000 -0600
@@ -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
+ *
  * 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 -pur linux-2.6.16/include/linux/fs.h linux-2.6.16.patched/include/linux/fs.h
--- linux-2.6.16/include/linux/fs.h     2006-03-19 23:53:29.000000000 -0600
+++ linux-2.6.16.patched/include/linux/fs.h     2006-03-24 10:27:20.000000000 -0600
@@ -83,6 +83,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

Re: [patch 2.6.16] loop.c to use write ops for fs requiring special locking [try #2]

[Anton Altaparmakov]

On Fri, 24 Mar 2006, Robert S Peterson wrote:
> On Sat, 2006-03-11 at 00:36 +0000, Anton Altaparmakov wrote:
> > On Fri, 10 Mar 2006, Andrew Morton wrote:
> > > When we've remembered what Al's statement meant in
> > > http://marc.theaimsgroup.com/?t=102129995600002&r=1&w=2
> > This is no longer relevant because my patch which is in current 2.6 
> > kernels (can't remember when I wrote it/when it got put into 2.6) did the 
> > conversion from prepare_write/commit_write to file ->write correctly so 
> > the data transformation still happens so that crypto still works with the 
> > loop driver.
> > 
> > To get back to Robert's patch that he is requesting to be included.  I 
> > think it is fine but the flag name could perhaps be better.  Perhaps 
> > "FS_AOPS_PRIVATE" or "FS_AOPS_SPECIAL" or "FS_AOPS_NEED_LOCKING" or 
> > even "FS_AOPS_REQUIRE_LOCKING" or something.  "FS_REQUIRES_LOCKING" just 
> > does not mean much and certainly would not suggest to me that no-one 
> > outside the file system should use the address space operations of the 
> > file system...  But maybe I am just bein picky.  (-:
> 
> Here is a resubmission of my patch to loop.c, this time against the
> 2.6.16 kernel.
> 
> Andrew: Sounds like Anton answered your concerns.
> Anton:  As per your suggestion, I changed the constant to your suggested
> FS_AOPS_NEED_LOCKING.

Looks good.

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

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

[Robert S Peterson]

Use normal write file operations rather than AOPS prepare_write and
commit_write when the backing filesystem requires special locking.

Signed-off-by:  Robert Peterson <rpeterso@redhat.com>

---

 drivers/block/loop.c |    9 ++++++++-
 include/linux/fs.h   |    1 +
 2 files changed, 9 insertions(+), 1 deletions(-)

121d2e76ae4b3f7ca3741e410664e138db5e1b13
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 9c3b94e..c762e76 100644
--- 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
+ *
  * 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
--
1.2.4

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

[Andrew Morton]

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.

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

[Christoph Hellwig]

On Mon, Mar 27, 2006 at 03:52:02PM -0600, Robert S Peterson wrote:
> Use normal write file operations rather than AOPS prepare_write and
> commit_write when the backing filesystem requires special locking.

NACK.  Adding random flags just makes the kernel unmaintainble.  The
right thing is to define a proper highlevel interface that can be
implemented properly on all filesystems plus a library helper for
normal pagecache-based filesystems using the aops.  There's already
various in-kernel filesystems that would require additional locking
or that aren't pagecache-based at all, please fix them up as part
of the patch(-series).

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

[Robert S Peterson]

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.

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

[Robert S Peterson]

On Tue, 2006-03-28 at 15:40 +0100, Christoph Hellwig wrote:
> NACK.  Adding random flags just makes the kernel unmaintainble.  The

Wrong on two accounts:  (1) This is NOT a random flag and (2) Adding
flags in fact can increase the maintainability of the kernel.  
With this new flag, other kernel drivers may now be written that
better understand the special needs of the underlying filesystem
and act accordingly.  Would you rather have modules like loop.c
special-case each underlying fs?  That's just plain ugly.  As in:

if underlying == ext2 { do this }
else if underlying == vfat { do that }
else if underlying == reiserfs { do the other }
etc.

> right thing is to define a proper highlevel interface that can be
> implemented properly on all filesystems plus a library helper for
> normal pagecache-based filesystems using the aops.  There's already
> various in-kernel filesystems that would require additional locking
> or that aren't pagecache-based at all, please fix them up as part
> of the patch(-series).

This is way out of the scope of the problem.  The problem is that
loop.c circumvents proper locking by going directly to prepare_write
rather than following the normal process.  If I added some kind of
library callbacks to allow cluster locking, it would break the
normal locking sequence of an ordinary write.

The normal sequence of an ordinary write typically looks like:

1. write
2.   (Take care of cluster locking if necessary)
3.   generic_file_write_nolock
4.      generic_file_aio_write_nolock
5.         generic_file_buffered_write
6.            a_ops->prepare_write
etc.

Right now, loop.c is circumventing step 2 (optional cluster locking
if needed by the underlying fs) by bypassing the "write" op.  If I 
added callbacks to prepare_write at (7) as you suggest, it would either
(a) introduce deadlocks conflicting with step 2 above or
(b) require the underlying fs to use its own versions of 3,4,5, and 6,
thus bypassing a great deal of vfs, which is not good for anyone.

Some could argue that loop.c should always use write rather than
prepare_write/commit_write, but most fs's don't have special
locking needs and therefore using them is a performance boost.
So why punish all fs's because of special locking needs require by
a few when we can simply communicate this need with a constant?

Bob Peterson
Red Hat Cluster Suite

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

[Andrew Morton]

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.

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

[Christoph Hellwig]

On Tue, Mar 28, 2006 at 09:59:17AM -0600, Robert S Peterson wrote:
> On Tue, 2006-03-28 at 15:40 +0100, Christoph Hellwig wrote:
> > NACK.  Adding random flags just makes the kernel unmaintainble.  The
> 
> Wrong on two accounts:  (1) This is NOT a random flag and (2) Adding
> flags in fact can increase the maintainability of the kernel.  
> With this new flag, other kernel drivers may now be written that
> better understand the special needs of the underlying filesystem
> and act accordingly.

adding flags adds special cases.  in this particular case it adds a special
case to hack around a leaking abstraction.  the right thing is to fix that
leaky abstraction as I said in my previous mail.  please go ahead and add 
a proper abstraction at the file operation level that
gets rid of this leaky abstraction instead of adding a kludge ontop of an
existing one.

> Would you rather have modules like loop.c
> special-case each underlying fs?  That's just plain ugly.  As in:
> 
> if underlying == ext2 { do this }
> else if underlying == vfat { do that }
> else if underlying == reiserfs { do the other }
> etc.

such crap might be acceptable inside redhat, but in kernel land it never
was so this never would be even considered an option.

> 
> > right thing is to define a proper highlevel interface that can be
> > implemented properly on all filesystems plus a library helper for
> > normal pagecache-based filesystems using the aops.  There's already
> > various in-kernel filesystems that would require additional locking
> > or that aren't pagecache-based at all, please fix them up as part
> > of the patch(-series).
> 
> This is way out of the scope of the problem.  The problem is that
> loop.c circumvents proper locking by going directly to prepare_write
> rather than following the normal process.  If I added some kind of
> library callbacks to allow cluster locking, it would break the
> normal locking sequence of an ordinary write.

no it's not out of scope.  we prefer to do things right, and kludging
a nother hack ontop of a historical accident is not right.  and given
that you care about getting rid of the old wart we have pretty nice
power to get you to do the right thing or nothing at all.

> 
> The normal sequence of an ordinary write typically looks like:

thanks, I'm not stupid, in fact I've writen quite a bit of that code.

please don't try to tell me the obvious things and go and fix the thing.

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

[Robert S Peterson]

On Wed, 2006-03-29 at 10:05 +0100, Christoph Hellwig wrote:
> adding flags adds special cases.  in this particular case it adds a special
> case to hack around a leaking abstraction.  the right thing is to fix that
> leaky abstraction as I said in my previous mail.  please go ahead and add 
> a proper abstraction at the file operation level that
> gets rid of this leaky abstraction instead of adding a kludge ontop of an
> existing one.

I considered doing this, but as I said, it would require the underlying
fs to use its own versions of 3,4,5, and 6, thus bypassing a great deal
of vfs.  Replacing 3,4,5 and 6 is certainly an option, but why change
200 lines of code when a problem can simply be fixed by 1 line of
code?  That seems like exposing a lot of people to a lot of unnecessary
risk to me.  I just can't justify replacing my car's entire engine
because one spark plug is misfiring.

loop.c already has the capability to use the write method rather than
the prepare_write/commit_write method.  So what is so wrong with giving
the underlying fs the ability to decide which with a flag?
My patch already received a thumbs up from Anton Altaparmakov, and I've
discussed the matter with Heinz M. as well, both of whom have changed
loop.c.

One flag does not make the kernel "unmaintainable."  I strongly believe
that the "right thing to do" in this case is to add this constant.

> such crap might be acceptable inside redhat, but in kernel land it never
> was so this never would be even considered an option.

Christoph, I would have hoped that a man of your obvious intelligence
would not resort to name calling, especially in the open-source
Community (with a capital C).  I expected more from you.

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

[Christoph Hellwig]

You still don't get it.

Calling address space operations directly from generic code is a layering
violation.  This got added during late 2.3/2.4 series to get loop working
again at all.  It's a pretty bad design in general.  We fixed the read side
by adding ->sendfile, and something similar is still needed on the write
side.  We got along with the broken write side because it kinda worked.

It doesn't work for your out of tree filesystem which isn't a problem per
se.  If you want it fixed the only way to get there is to fix it for real
instead of adding another unmaintainable cludge.  So either go ahead and
add a new file operation that is the counterpart to ->sendfile or go away.

And please take a look at Jens Axboe's splice patch, I suspect you could
easily piggyback on that.