Btrfs devel - Apr 2012 - [PATCH] Btrfs: do not do filemap_write_and_wait_range in fsync

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

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html