Btrfs: do not backup tree roots when fsync
diff mbox

Message ID 20170913182521.31304-1-bo.li.liu@oracle.com
State New
Headers show

Commit Message

Liu Bo Sept. 13, 2017, 6:25 p.m. UTC
It doens't make sense to backup tree roots when doing fsync, since
during fsync those tree roots have not been consistent on disk.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/disk-io.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Qu Wenruo Sept. 14, 2017, 1:55 a.m. UTC | #1
On 2017年09月14日 02:25, Liu Bo wrote:
> It doens't make sense to backup tree roots when doing fsync, since
> during fsync those tree roots have not been consistent on disk.
> 
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>

Reviewed-by: Qu Wenruo <quwenruo.btrfs@gmx.com>

With a pit can be improved.
> ---
>   fs/btrfs/disk-io.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 79ac228..a145a88 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3668,7 +3668,14 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
>   	u64 flags;
>   
>   	do_barriers = !btrfs_test_opt(fs_info, NOBARRIER);
> -	backup_super_roots(fs_info);
> +
> +	/*
> +	 * max_mirrors == 0 indicates we're from commit_transaction,
> +	 * not from fsync where the tree roots in fs_info have not
> +	 * been consistent on disk.
> +	 */
> +	if (max_mirrors == 0)
> +		backup_super_roots(fs_info);

BTW, the @max_mirrors naming here is really confusing.
Normally I would expect max_mirrors == 0 means we don't need to backup 
super roots...

And since there are only two callers it won't be a big thing to change.

Thanks,
Qu
>   
>   	sb = fs_info->super_for_commit;
>   	dev_item = &sb->dev_item;
> 
--
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
David Sterba Sept. 14, 2017, 12:49 p.m. UTC | #2
On Thu, Sep 14, 2017 at 09:55:48AM +0800, Qu Wenruo wrote:
> 
> 
> On 2017年09月14日 02:25, Liu Bo wrote:
> > It doens't make sense to backup tree roots when doing fsync, since
> > during fsync those tree roots have not been consistent on disk.
> > 
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> 
> Reviewed-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
> 
> With a pit can be improved.
> > ---
> >   fs/btrfs/disk-io.c | 9 ++++++++-
> >   1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > index 79ac228..a145a88 100644
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -3668,7 +3668,14 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
> >   	u64 flags;
> >   
> >   	do_barriers = !btrfs_test_opt(fs_info, NOBARRIER);
> > -	backup_super_roots(fs_info);
> > +
> > +	/*
> > +	 * max_mirrors == 0 indicates we're from commit_transaction,
> > +	 * not from fsync where the tree roots in fs_info have not
> > +	 * been consistent on disk.
> > +	 */
> > +	if (max_mirrors == 0)
> > +		backup_super_roots(fs_info);
> 
> BTW, the @max_mirrors naming here is really confusing.
> Normally I would expect max_mirrors == 0 means we don't need to backup 
> super roots...

Agreed it's confusing, could be something like "bool write_backups" (in a
separate patch).
--
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
Liu Bo Sept. 15, 2017, 9:09 p.m. UTC | #3
On Thu, Sep 14, 2017 at 02:49:03PM +0200, David Sterba wrote:
> On Thu, Sep 14, 2017 at 09:55:48AM +0800, Qu Wenruo wrote:
> > 
> > 
> > On 2017年09月14日 02:25, Liu Bo wrote:
> > > It doens't make sense to backup tree roots when doing fsync, since
> > > during fsync those tree roots have not been consistent on disk.
> > > 
> > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > 
> > Reviewed-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
> > 
> > With a pit can be improved.
> > > ---
> > >   fs/btrfs/disk-io.c | 9 ++++++++-
> > >   1 file changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > > index 79ac228..a145a88 100644
> > > --- a/fs/btrfs/disk-io.c
> > > +++ b/fs/btrfs/disk-io.c
> > > @@ -3668,7 +3668,14 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
> > >   	u64 flags;
> > >   
> > >   	do_barriers = !btrfs_test_opt(fs_info, NOBARRIER);
> > > -	backup_super_roots(fs_info);
> > > +
> > > +	/*
> > > +	 * max_mirrors == 0 indicates we're from commit_transaction,
> > > +	 * not from fsync where the tree roots in fs_info have not
> > > +	 * been consistent on disk.
> > > +	 */
> > > +	if (max_mirrors == 0)
> > > +		backup_super_roots(fs_info);
> > 
> > BTW, the @max_mirrors naming here is really confusing.
> > Normally I would expect max_mirrors == 0 means we don't need to backup 
> > super roots...
> 
> Agreed it's confusing, could be something like "bool write_backups" (in a
> separate patch).

Good point, will do in a separate one, thanks to both for the
comments.

Thanks,

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

Patch
diff mbox

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 79ac228..a145a88 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3668,7 +3668,14 @@  int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
 	u64 flags;
 
 	do_barriers = !btrfs_test_opt(fs_info, NOBARRIER);
-	backup_super_roots(fs_info);
+
+	/*
+	 * max_mirrors == 0 indicates we're from commit_transaction,
+	 * not from fsync where the tree roots in fs_info have not
+	 * been consistent on disk.
+	 */
+	if (max_mirrors == 0)
+		backup_super_roots(fs_info);
 
 	sb = fs_info->super_for_commit;
 	dev_item = &sb->dev_item;