[PATCH] Btrfs: do not do filemap_write_and_wait_range in fsync

[PATCH] Btrfs: do not do filemap_write_and_wait_range in fsync

[Josef Bacik]

We already do the btrfs_wait_ordered_range which will do this for us, so
just remove this call so we don't call it twice.  Thanks,

Signed-off-by: Josef Bacik <josef@redhat.com>
---
 fs/btrfs/file.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index f0da02b..0c8ed6a 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1492,14 +1492,15 @@ int btrfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 
 	trace_btrfs_sync_file(file, datasync);
 
-	ret = filemap_write_and_wait_range(inode->i_mapping, start, end);
-	if (ret)
-		return ret;
 	mutex_lock(&inode->i_mutex);
 
-	/* we wait first, since the writeback may change the inode */
+	/*
+	 * we wait first, since the writeback may change the inode, also wait
+	 * ordered range does a filemape_write_and_wait_range which is why we
+	 * don't do it above like other file systems.
+	 */
 	root->log_batch++;
-	btrfs_wait_ordered_range(inode, 0, (u64)-1);
+	btrfs_wait_ordered_range(inode, start, end);
 	root->log_batch++;
 
 	/*
-- 
1.7.7.6

Re: [PATCH] Btrfs: do not do filemap_write_and_wait_range in fsync

[Chris Mason]

On Mon, Apr 23, 2012 at 03:06:41PM -0400, Josef Bacik wrote:
> We already do the btrfs_wait_ordered_range which will do this for us, so
> just remove this call so we don't call it twice.  Thanks,
> 
> Signed-off-by: Josef Bacik <josef@redhat.com>
> ---
>  fs/btrfs/file.c |   11 ++++++-----
>  1 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index f0da02b..0c8ed6a 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1492,14 +1492,15 @@ int btrfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
>  
>  	trace_btrfs_sync_file(file, datasync);
>  
> -	ret = filemap_write_and_wait_range(inode->i_mapping, start, end);
> -	if (ret)
> -		return ret;
>  	mutex_lock(&inode->i_mutex);

Hmmm, I think there are some good benefits to doing the wait
outside of the mutex.

The question is, do we need to do it again after we take the mutex?  I
tend to think no, if you're racing another write with the fsync, you get
what you deserve.

-chris

Re: [PATCH] Btrfs: do not do filemap_write_and_wait_range in fsync

[Josef Bacik]

On Mon, Apr 23, 2012 at 07:29:45PM -0400, Chris Mason wrote:
> On Mon, Apr 23, 2012 at 03:06:41PM -0400, Josef Bacik wrote:
> > We already do the btrfs_wait_ordered_range which will do this for us, so
> > just remove this call so we don't call it twice.  Thanks,
> > 
> > Signed-off-by: Josef Bacik <josef@redhat.com>
> > ---
> >  fs/btrfs/file.c |   11 ++++++-----
> >  1 files changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> > index f0da02b..0c8ed6a 100644
> > --- a/fs/btrfs/file.c
> > +++ b/fs/btrfs/file.c
> > @@ -1492,14 +1492,15 @@ int btrfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
> >  
> >  	trace_btrfs_sync_file(file, datasync);
> >  
> > -	ret = filemap_write_and_wait_range(inode->i_mapping, start, end);
> > -	if (ret)
> > -		return ret;
> >  	mutex_lock(&inode->i_mutex);
> 
> Hmmm, I think there are some good benefits to doing the wait
> outside of the mutex.
> 
> The question is, do we need to do it again after we take the mutex?  I
> tend to think no, if you're racing another write with the fsync, you get
> what you deserve.
> 

Yeah I thought about this and I wasn't sure, the only reason I left it where it
was is because of the log_batch thing, I'm not sure what exactly that does and I
didn't want to mess with what we have.  If that can be done outside the mutex
then lets do that.  Thanks,

Josef

Re: [PATCH] Btrfs: do not do filemap_write_and_wait_range in fsync

[Chris Mason]

On Tue, Apr 24, 2012 at 09:14:47AM -0400, Josef Bacik wrote:
> On Mon, Apr 23, 2012 at 07:29:45PM -0400, Chris Mason wrote:
> > On Mon, Apr 23, 2012 at 03:06:41PM -0400, Josef Bacik wrote:
> > > We already do the btrfs_wait_ordered_range which will do this for us, so
> > > just remove this call so we don't call it twice.  Thanks,
> > > 
> > > Signed-off-by: Josef Bacik <josef@redhat.com>
> > > ---
> > >  fs/btrfs/file.c |   11 ++++++-----
> > >  1 files changed, 6 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> > > index f0da02b..0c8ed6a 100644
> > > --- a/fs/btrfs/file.c
> > > +++ b/fs/btrfs/file.c
> > > @@ -1492,14 +1492,15 @@ int btrfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
> > >  
> > >  	trace_btrfs_sync_file(file, datasync);
> > >  
> > > -	ret = filemap_write_and_wait_range(inode->i_mapping, start, end);
> > > -	if (ret)
> > > -		return ret;
> > >  	mutex_lock(&inode->i_mutex);
> > 
> > Hmmm, I think there are some good benefits to doing the wait
> > outside of the mutex.
> > 
> > The question is, do we need to do it again after we take the mutex?  I
> > tend to think no, if you're racing another write with the fsync, you get
> > what you deserve.
> > 
> 
> Yeah I thought about this and I wasn't sure, the only reason I left it where it
> was is because of the log_batch thing, I'm not sure what exactly that does and I
> didn't want to mess with what we have.  If that can be done outside the mutex
> then lets do that.  Thanks,

The log batch is just something to kick the logging code so that it
knows there are more loggers coming in.  It's the wait around for more
changes thing.

-chris