[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/