[PATCH] Fix bug in FUSE where the attribute cache for a file was not cleared when a file is opened with O_TRUNC.

[PATCH] Fix bug in FUSE where the attribute cache for a file was not cleared when a file is opened with O_TRUNC.

[Ken Sumrall]

Signed-off-by: Ken Sumrall <ksumrall@android.com>
---
 fs/fuse/file.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index c822458..4fbe62c 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -134,6 +134,7 @@ EXPORT_SYMBOL_GPL(fuse_do_open);
 void fuse_finish_open(struct inode *inode, struct file *file)
 {
 	struct fuse_file *ff = file->private_data;
+	struct fuse_conn *fc = get_fuse_conn(inode);
 
 	if (ff->open_flags & FOPEN_DIRECT_IO)
 		file->f_op = &fuse_direct_io_file_operations;
@@ -141,6 +142,8 @@ void fuse_finish_open(struct inode *inode, struct file *file)
 		invalidate_inode_pages2(inode->i_mapping);
 	if (ff->open_flags & FOPEN_NONSEEKABLE)
 		nonseekable_open(inode, file);
+	if (fc->atomic_o_trunc && (file->f_flags & O_TRUNC))
+		fuse_invalidate_attr(inode);
 }
 
 int fuse_open_common(struct inode *inode, struct file *file, bool isdir)
-- 
1.7.3.1

Re: [PATCH] Fix bug in FUSE where the attribute cache for a file was not cleared when a file is opened with O_TRUNC.

[Andrew Morton]

On Fri, 29 Oct 2010 16:41:40 -0700 Ken Sumrall <ksumrall@android.com> wrote:

> Signed-off-by: Ken Sumrall <ksumrall@android.com>
> ---
>  fs/fuse/file.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index c822458..4fbe62c 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -134,6 +134,7 @@ EXPORT_SYMBOL_GPL(fuse_do_open);
>  void fuse_finish_open(struct inode *inode, struct file *file)
>  {
>  	struct fuse_file *ff = file->private_data;
> +	struct fuse_conn *fc = get_fuse_conn(inode);
>  
>  	if (ff->open_flags & FOPEN_DIRECT_IO)
>  		file->f_op = &fuse_direct_io_file_operations;
> @@ -141,6 +142,8 @@ void fuse_finish_open(struct inode *inode, struct file *file)
>  		invalidate_inode_pages2(inode->i_mapping);
>  	if (ff->open_flags & FOPEN_NONSEEKABLE)
>  		nonseekable_open(inode, file);
> +	if (fc->atomic_o_trunc && (file->f_flags & O_TRUNC))
> +		fuse_invalidate_attr(inode);
>  }
>  
>  int fuse_open_common(struct inode *inode, struct file *file, bool isdir)

What were the user-visible effects of this bug?

Fwd: [PATCH] Fix bug in FUSE where the attribute cache for a file was not cleared when a file is opened with O_TRUNC.

[Ken Sumrall]

Resending to the list, as it was rejected due to html content.



---------- Forwarded message ----------
From: Ken Sumrall <ksumrall@android.com>
Date: Fri, Oct 29, 2010 at 5:31 PM
Subject: Re: [PATCH] Fix bug in FUSE where the attribute cache for a
file was not cleared when a file is opened with O_TRUNC.
To: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org, Miklos Szeredi <miklos@szeredi.hu>,
Jens Axboe <jens.axboe@oracle.com>, Anfei <anfei.zhou@gmail.com>,
"Anand V. Avati" <avati@gluster.com>, fuse-devel@lists.sourceforge.net


Here is a snippet of code from a test case we have:
  {
   FILE*   pFile = NULL;
   int32_t data  = 0x01020304;
   LOGE("WRITETEST start");
   pFile = fopen("/sdcard/write_test", "wb");
   if( NULL != pFile ) {
       LOGE("WRITETEST : starting position %d", ftell(pFile));
       fseek(pFile, 0, SEEK_END);
       LOGE("WRITETEST : starting size %d", ftell(pFile));
       fwrite(&data, sizeof(int32_t), 1, pFile);
       fclose(pFile);
   } else {
       LOGE("WRITETEST : failed to open the file");
   }
   LOGE("WRITETEST end");
}
fopen("wb") turns into open(O_TRUNC) in bionic (the Android libc).
The code is odd in that it then does fseek(SEEK_END) on a newly truncated file.
However, that is completely legal, and should set the file pointer to offset 0.
If the file had previously existed, it's attributes are still in the
FUSE attribute
cache, so the fseek(SEEK_END) positions the file pointer to the end
size of the file
before it was truncated.
This has probably never been seen before because most code that opens
with O_TRUNC
just starts writing, and doesn't query the attributes of the file till
after a write.  Writing clears
the attribute cache for the file so it is fetched from the filesystem
the next time it is
requested.
___
Ken
On Fri, Oct 29, 2010 at 5:02 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
>
> On Fri, 29 Oct 2010 16:41:40 -0700 Ken Sumrall <ksumrall@android.com> wrote:
>
> > Signed-off-by: Ken Sumrall <ksumrall@android.com>
> > ---
> >  fs/fuse/file.c |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> >
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > index c822458..4fbe62c 100644
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> > @@ -134,6 +134,7 @@ EXPORT_SYMBOL_GPL(fuse_do_open);
> >  void fuse_finish_open(struct inode *inode, struct file *file)
> >  {
> >       struct fuse_file *ff = file->private_data;
> > +     struct fuse_conn *fc = get_fuse_conn(inode);
> >
> >       if (ff->open_flags & FOPEN_DIRECT_IO)
> >               file->f_op = &fuse_direct_io_file_operations;
> > @@ -141,6 +142,8 @@ void fuse_finish_open(struct inode *inode, struct file *file)
> >               invalidate_inode_pages2(inode->i_mapping);
> >       if (ff->open_flags & FOPEN_NONSEEKABLE)
> >               nonseekable_open(inode, file);
> > +     if (fc->atomic_o_trunc && (file->f_flags & O_TRUNC))
> > +             fuse_invalidate_attr(inode);
> >  }
> >
> >  int fuse_open_common(struct inode *inode, struct file *file, bool isdir)
>
> What were the user-visible effects of this bug?
>

Re: [PATCH] Fix bug in FUSE where the attribute cache for a file was not cleared when a file is opened with O_TRUNC.

[Miklos Szeredi]

Ken,

Thanks for the patch.

Here's a slightly updated one.  Does this also look OK?

Miklos


----
Subject: fuse: fix attributes after open(O_TRUNC)
From: Ken Sumrall <ksumrall@android.com>

The attribute cache for a file was not being cleared when a file is
opened with O_TRUNC.

If the filesystem's open operation truncates the file
("atomic_o_trunc" feature flag is set) then the kernel should
invalidate the cached st_mtime and st_ctime attributes.

Also i_size should be explicitly be set to zero as it is used
sometimes without refreshing the cache.

Signed-off-by: Ken Sumrall <ksumrall@android.com>
Cc: Anfei <anfei.zhou@gmail.com>
Cc: "Anand V. Avati" <avati@gluster.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Miklos Szeredi <miklos@szeredi.hu>
Cc: stable@kernel.org
---

 fs/fuse/file.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

Index: linux-2.6/fs/fuse/file.c
===================================================================
--- linux-2.6.orig/fs/fuse/file.c	2010-10-27 14:04:02.000000000 +0200
+++ linux-2.6/fs/fuse/file.c	2010-10-30 13:14:07.000000000 +0200
@@ -134,6 +134,7 @@ EXPORT_SYMBOL_GPL(fuse_do_open);
 void fuse_finish_open(struct inode *inode, struct file *file)
 {
 	struct fuse_file *ff = file->private_data;
+	struct fuse_conn *fc = get_fuse_conn(inode);
 
 	if (ff->open_flags & FOPEN_DIRECT_IO)
 		file->f_op = &fuse_direct_io_file_operations;
@@ -141,6 +142,15 @@ void fuse_finish_open(struct inode *inod
 		invalidate_inode_pages2(inode->i_mapping);
 	if (ff->open_flags & FOPEN_NONSEEKABLE)
 		nonseekable_open(inode, file);
+	if (fc->atomic_o_trunc && (file->f_flags & O_TRUNC)) {
+		struct fuse_inode *fi = get_fuse_inode(inode);
+
+		spin_lock(&fc->lock);
+		fi->attr_version = ++fc->attr_version;
+		i_size_write(inode, 0);
+		spin_unlock(&fc->lock);
+		fuse_invalidate_attr(inode);
+	}
 }
 
 int fuse_open_common(struct inode *inode, struct file *file, bool isdir)

Re: [PATCH] Fix bug in FUSE where the attribute cache for a file was not cleared when a file is opened with O_TRUNC.

[Ken Sumrall]

I'm no FUSE expert, so I don't feel qualified to fully evaluate the
changes, but the
locking looks correct and non-blocking, and the code still passes our
test case and
doesn't crash, so it gets my vote of approval.

___
Ken


On Sat, Oct 30, 2010 at 4:31 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> Ken,
>
> Thanks for the patch.
>
> Here's a slightly updated one.  Does this also look OK?
>
> Miklos
>
>
> ----
> Subject: fuse: fix attributes after open(O_TRUNC)
> From: Ken Sumrall <ksumrall@android.com>
>
> The attribute cache for a file was not being cleared when a file is
> opened with O_TRUNC.
>
> If the filesystem's open operation truncates the file
> ("atomic_o_trunc" feature flag is set) then the kernel should
> invalidate the cached st_mtime and st_ctime attributes.
>
> Also i_size should be explicitly be set to zero as it is used
> sometimes without refreshing the cache.
>
> Signed-off-by: Ken Sumrall <ksumrall@android.com>
> Cc: Anfei <anfei.zhou@gmail.com>
> Cc: "Anand V. Avati" <avati@gluster.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Miklos Szeredi <miklos@szeredi.hu>
> Cc: stable@kernel.org
> ---
>
>  fs/fuse/file.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> Index: linux-2.6/fs/fuse/file.c
> ===================================================================
> --- linux-2.6.orig/fs/fuse/file.c       2010-10-27 14:04:02.000000000 +0200
> +++ linux-2.6/fs/fuse/file.c    2010-10-30 13:14:07.000000000 +0200
> @@ -134,6 +134,7 @@ EXPORT_SYMBOL_GPL(fuse_do_open);
>  void fuse_finish_open(struct inode *inode, struct file *file)
>  {
>        struct fuse_file *ff = file->private_data;
> +       struct fuse_conn *fc = get_fuse_conn(inode);
>
>        if (ff->open_flags & FOPEN_DIRECT_IO)
>                file->f_op = &fuse_direct_io_file_operations;
> @@ -141,6 +142,15 @@ void fuse_finish_open(struct inode *inod
>                invalidate_inode_pages2(inode->i_mapping);
>        if (ff->open_flags & FOPEN_NONSEEKABLE)
>                nonseekable_open(inode, file);
> +       if (fc->atomic_o_trunc && (file->f_flags & O_TRUNC)) {
> +               struct fuse_inode *fi = get_fuse_inode(inode);
> +
> +               spin_lock(&fc->lock);
> +               fi->attr_version = ++fc->attr_version;
> +               i_size_write(inode, 0);
> +               spin_unlock(&fc->lock);
> +               fuse_invalidate_attr(inode);
> +       }
>  }
>
>  int fuse_open_common(struct inode *inode, struct file *file, bool isdir)
>

Re: [PATCH] Fix bug in FUSE where the attribute cache for a file was not cleared when a file is opened with O_TRUNC.

[Andrew Morton]

On Sat, 30 Oct 2010 13:31:23 +0200
Miklos Szeredi <miklos@szeredi.hu> wrote:

> Subject: fuse: fix attributes after open(O_TRUNC)
> From: Ken Sumrall <ksumrall@android.com>
> 
> The attribute cache for a file was not being cleared when a file is
> opened with O_TRUNC.
> 
> If the filesystem's open operation truncates the file
> ("atomic_o_trunc" feature flag is set) then the kernel should
> invalidate the cached st_mtime and st_ctime attributes.
> 
> Also i_size should be explicitly be set to zero as it is used
> sometimes without refreshing the cache.
> 
> Signed-off-by: Ken Sumrall <ksumrall@android.com>
> Cc: Anfei <anfei.zhou@gmail.com>
> Cc: "Anand V. Avati" <avati@gluster.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Miklos Szeredi <miklos@szeredi.hu>
> Cc: stable@kernel.org
> ---
> 
>  fs/fuse/file.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> Index: linux-2.6/fs/fuse/file.c
> ===================================================================
> --- linux-2.6.orig/fs/fuse/file.c	2010-10-27 14:04:02.000000000 +0200
> +++ linux-2.6/fs/fuse/file.c	2010-10-30 13:14:07.000000000 +0200
> @@ -134,6 +134,7 @@ EXPORT_SYMBOL_GPL(fuse_do_open);
>  void fuse_finish_open(struct inode *inode, struct file *file)
>  {
>  	struct fuse_file *ff = file->private_data;
> +	struct fuse_conn *fc = get_fuse_conn(inode);
>  
>  	if (ff->open_flags & FOPEN_DIRECT_IO)
>  		file->f_op = &fuse_direct_io_file_operations;
> @@ -141,6 +142,15 @@ void fuse_finish_open(struct inode *inod
>  		invalidate_inode_pages2(inode->i_mapping);
>  	if (ff->open_flags & FOPEN_NONSEEKABLE)
>  		nonseekable_open(inode, file);
> +	if (fc->atomic_o_trunc && (file->f_flags & O_TRUNC)) {
> +		struct fuse_inode *fi = get_fuse_inode(inode);
> +
> +		spin_lock(&fc->lock);
> +		fi->attr_version = ++fc->attr_version;
> +		i_size_write(inode, 0);

i_size_write() requires that i_mutex be held, as described at the
i_size_write() definition site.

Is it reliably the case that i_mutex is held here?

> +		spin_unlock(&fc->lock);
> +		fuse_invalidate_attr(inode);
> +	}
>  }
>  
>  int fuse_open_common(struct inode *inode, struct file *file, bool isdir)

Re: [PATCH] Fix bug in FUSE where the attribute cache for a file was not cleared when a file is opened with O_TRUNC.

[Miklos Szeredi]

On Thu, 11 Nov 2010, Andrew Morton wrote:
> On Sat, 30 Oct 2010 13:31:23 +0200
> Miklos Szeredi <miklos@szeredi.hu> wrote:
> 
> > Subject: fuse: fix attributes after open(O_TRUNC)
> > From: Ken Sumrall <ksumrall@android.com>
> > 
> > The attribute cache for a file was not being cleared when a file is
> > opened with O_TRUNC.
> > 
> > If the filesystem's open operation truncates the file
> > ("atomic_o_trunc" feature flag is set) then the kernel should
> > invalidate the cached st_mtime and st_ctime attributes.
> > 
> > Also i_size should be explicitly be set to zero as it is used
> > sometimes without refreshing the cache.
> > 
> > Signed-off-by: Ken Sumrall <ksumrall@android.com>
> > Cc: Anfei <anfei.zhou@gmail.com>
> > Cc: "Anand V. Avati" <avati@gluster.com>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > Signed-off-by: Miklos Szeredi <miklos@szeredi.hu>
> > Cc: stable@kernel.org
> > ---
> > 
> >  fs/fuse/file.c |   10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > Index: linux-2.6/fs/fuse/file.c
> > ===================================================================
> > --- linux-2.6.orig/fs/fuse/file.c	2010-10-27 14:04:02.000000000 +0200
> > +++ linux-2.6/fs/fuse/file.c	2010-10-30 13:14:07.000000000 +0200
> > @@ -134,6 +134,7 @@ EXPORT_SYMBOL_GPL(fuse_do_open);
> >  void fuse_finish_open(struct inode *inode, struct file *file)
> >  {
> >  	struct fuse_file *ff = file->private_data;
> > +	struct fuse_conn *fc = get_fuse_conn(inode);
> >  
> >  	if (ff->open_flags & FOPEN_DIRECT_IO)
> >  		file->f_op = &fuse_direct_io_file_operations;
> > @@ -141,6 +142,15 @@ void fuse_finish_open(struct inode *inod
> >  		invalidate_inode_pages2(inode->i_mapping);
> >  	if (ff->open_flags & FOPEN_NONSEEKABLE)
> >  		nonseekable_open(inode, file);
> > +	if (fc->atomic_o_trunc && (file->f_flags & O_TRUNC)) {
> > +		struct fuse_inode *fi = get_fuse_inode(inode);
> > +
> > +		spin_lock(&fc->lock);
> > +		fi->attr_version = ++fc->attr_version;
> > +		i_size_write(inode, 0);
> 
> i_size_write() requires that i_mutex be held, as described at the
> i_size_write() definition site.
> 
> Is it reliably the case that i_mutex is held here?

No.  AFAICS any lock will do, not just i_mutex.  Fuse consistently
uses fc->lock to protect i_size_write().

Thanks,
Miklos