All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Btrfs: do not backup tree roots when fsync
@ 2017-09-13 18:25 Liu Bo
  2017-09-14  1:55 ` Qu Wenruo
  2017-09-22 23:36 ` [PATCH] Btrfs: use self-explaining variable Liu Bo
  0 siblings, 2 replies; 9+ messages in thread
From: Liu Bo @ 2017-09-13 18:25 UTC (permalink / raw)
  To: linux-btrfs

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(-)

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;
-- 
2.9.4


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH] Btrfs: do not backup tree roots when fsync
  2017-09-13 18:25 [PATCH] Btrfs: do not backup tree roots when fsync Liu Bo
@ 2017-09-14  1:55 ` Qu Wenruo
  2017-09-14 12:49   ` David Sterba
  2017-09-22 23:36 ` [PATCH] Btrfs: use self-explaining variable Liu Bo
  1 sibling, 1 reply; 9+ messages in thread
From: Qu Wenruo @ 2017-09-14  1:55 UTC (permalink / raw)
  To: Liu Bo, linux-btrfs



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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] Btrfs: do not backup tree roots when fsync
  2017-09-14  1:55 ` Qu Wenruo
@ 2017-09-14 12:49   ` David Sterba
  2017-09-15 21:09     ` Liu Bo
  0 siblings, 1 reply; 9+ messages in thread
From: David Sterba @ 2017-09-14 12:49 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: Liu Bo, linux-btrfs

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).

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] Btrfs: do not backup tree roots when fsync
  2017-09-14 12:49   ` David Sterba
@ 2017-09-15 21:09     ` Liu Bo
  0 siblings, 0 replies; 9+ messages in thread
From: Liu Bo @ 2017-09-15 21:09 UTC (permalink / raw)
  To: dsterba, Qu Wenruo, linux-btrfs

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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* [PATCH] Btrfs: use self-explaining variable
  2017-09-13 18:25 [PATCH] Btrfs: do not backup tree roots when fsync Liu Bo
  2017-09-14  1:55 ` Qu Wenruo
@ 2017-09-22 23:36 ` Liu Bo
  2017-09-23  0:46   ` Qu Wenruo
  1 sibling, 1 reply; 9+ messages in thread
From: Liu Bo @ 2017-09-22 23:36 UTC (permalink / raw)
  To: linux-btrfs

This uses a bool 'do_backup' to help understand this piece of code.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
This is based on a patch "Btrfs: do not backup tree roots when fsync".

 fs/btrfs/disk-io.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index cdb7043..9811b9d 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3691,6 +3691,7 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
 	int max_errors;
 	int total_errors = 0;
 	u64 flags;
+	bool do_backup = (max_mirrors == 0);
 
 	do_barriers = !btrfs_test_opt(fs_info, NOBARRIER);
 
@@ -3699,7 +3700,7 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
 	 * not from fsync where the tree roots in fs_info have not
 	 * been consistent on disk.
 	 */
-	if (max_mirrors == 0)
+	if (do_backup)
 		backup_super_roots(fs_info);
 
 	sb = fs_info->super_for_commit;
-- 
2.9.4


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH] Btrfs: use self-explaining variable
  2017-09-22 23:36 ` [PATCH] Btrfs: use self-explaining variable Liu Bo
@ 2017-09-23  0:46   ` Qu Wenruo
  2017-09-23  0:48     ` Liu Bo
  0 siblings, 1 reply; 9+ messages in thread
From: Qu Wenruo @ 2017-09-23  0:46 UTC (permalink / raw)
  To: Liu Bo, linux-btrfs



On 2017年09月23日 07:36, Liu Bo wrote:
> This uses a bool 'do_backup' to help understand this piece of code.
> 
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
> This is based on a patch "Btrfs: do not backup tree roots when fsync".
> 
>   fs/btrfs/disk-io.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index cdb7043..9811b9d 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3691,6 +3691,7 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
>   	int max_errors;
>   	int total_errors = 0;
>   	u64 flags;
> +	bool do_backup = (max_mirrors == 0);

Why not replacing @max_mirrors with @do_backup as parameter?

Thanks,
Qu
>   
>   	do_barriers = !btrfs_test_opt(fs_info, NOBARRIER);
>   
> @@ -3699,7 +3700,7 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
>   	 * not from fsync where the tree roots in fs_info have not
>   	 * been consistent on disk.
>   	 */
> -	if (max_mirrors == 0)
> +	if (do_backup)
>   		backup_super_roots(fs_info);
>   
>   	sb = fs_info->super_for_commit;
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] Btrfs: use self-explaining variable
  2017-09-23  0:46   ` Qu Wenruo
@ 2017-09-23  0:48     ` Liu Bo
  2017-09-23  1:09       ` Qu Wenruo
  0 siblings, 1 reply; 9+ messages in thread
From: Liu Bo @ 2017-09-23  0:48 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: linux-btrfs

On Sat, Sep 23, 2017 at 08:46:55AM +0800, Qu Wenruo wrote:
> 
> 
> On 2017年09月23日 07:36, Liu Bo wrote:
> > This uses a bool 'do_backup' to help understand this piece of code.
> > 
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > ---
> > This is based on a patch "Btrfs: do not backup tree roots when fsync".
> > 
> >   fs/btrfs/disk-io.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > index cdb7043..9811b9d 100644
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -3691,6 +3691,7 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
> >   	int max_errors;
> >   	int total_errors = 0;
> >   	u64 flags;
> > +	bool do_backup = (max_mirrors == 0);
> 
> Why not replacing @max_mirrors with @do_backup as parameter?

If I read the code correctly, max_mirrors is not just for deciding
backup.

thanks,

-liubo

> 
> Thanks,
> Qu
> >   	do_barriers = !btrfs_test_opt(fs_info, NOBARRIER);
> > @@ -3699,7 +3700,7 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
> >   	 * not from fsync where the tree roots in fs_info have not
> >   	 * been consistent on disk.
> >   	 */
> > -	if (max_mirrors == 0)
> > +	if (do_backup)
> >   		backup_super_roots(fs_info);
> >   	sb = fs_info->super_for_commit;
> > 

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] Btrfs: use self-explaining variable
  2017-09-23  0:48     ` Liu Bo
@ 2017-09-23  1:09       ` Qu Wenruo
  2017-09-24 13:24         ` David Sterba
  0 siblings, 1 reply; 9+ messages in thread
From: Qu Wenruo @ 2017-09-23  1:09 UTC (permalink / raw)
  To: bo.li.liu; +Cc: linux-btrfs



On 2017年09月23日 08:48, Liu Bo wrote:
> On Sat, Sep 23, 2017 at 08:46:55AM +0800, Qu Wenruo wrote:
>>
>>
>> On 2017年09月23日 07:36, Liu Bo wrote:
>>> This uses a bool 'do_backup' to help understand this piece of code.
>>>
>>> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
>>> ---
>>> This is based on a patch "Btrfs: do not backup tree roots when fsync".
>>>
>>>    fs/btrfs/disk-io.c | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>> index cdb7043..9811b9d 100644
>>> --- a/fs/btrfs/disk-io.c
>>> +++ b/fs/btrfs/disk-io.c
>>> @@ -3691,6 +3691,7 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
>>>    	int max_errors;
>>>    	int total_errors = 0;
>>>    	u64 flags;
>>> +	bool do_backup = (max_mirrors == 0);
>>
>> Why not replacing @max_mirrors with @do_backup as parameter?
> 
> If I read the code correctly, max_mirrors is not just for deciding
> backup.

That's strange.

write_all_supers() only uses @max_mirrors by passing it to 
write_dev_supers() and wait_dev_supers().

Both of the write/wait_dev_supers() will replace @max_mirrors to 
BTRFS_SUPER_MIRROR_MAX if it's zero.

Further more, all write_all_supers() callers just pass @max_mirrors as 
bool (either 0 or 1).

So I don't see any point not replacing the parameter as bool.

Thanks,
Qu
> 
> thanks,
> 
> -liubo
> 
>>
>> Thanks,
>> Qu
>>>    	do_barriers = !btrfs_test_opt(fs_info, NOBARRIER);
>>> @@ -3699,7 +3700,7 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
>>>    	 * not from fsync where the tree roots in fs_info have not
>>>    	 * been consistent on disk.
>>>    	 */
>>> -	if (max_mirrors == 0)
>>> +	if (do_backup)
>>>    		backup_super_roots(fs_info);
>>>    	sb = fs_info->super_for_commit;
>>>
> --
> 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
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] Btrfs: use self-explaining variable
  2017-09-23  1:09       ` Qu Wenruo
@ 2017-09-24 13:24         ` David Sterba
  0 siblings, 0 replies; 9+ messages in thread
From: David Sterba @ 2017-09-24 13:24 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: bo.li.liu, linux-btrfs

On Sat, Sep 23, 2017 at 09:09:24AM +0800, Qu Wenruo wrote:
> 
> 
> On 2017年09月23日 08:48, Liu Bo wrote:
> > On Sat, Sep 23, 2017 at 08:46:55AM +0800, Qu Wenruo wrote:
> >>
> >>
> >> On 2017年09月23日 07:36, Liu Bo wrote:
> >>> This uses a bool 'do_backup' to help understand this piece of code.
> >>>
> >>> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> >>> ---
> >>> This is based on a patch "Btrfs: do not backup tree roots when fsync".
> >>>
> >>>    fs/btrfs/disk-io.c | 3 ++-
> >>>    1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> >>> index cdb7043..9811b9d 100644
> >>> --- a/fs/btrfs/disk-io.c
> >>> +++ b/fs/btrfs/disk-io.c
> >>> @@ -3691,6 +3691,7 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
> >>>    	int max_errors;
> >>>    	int total_errors = 0;
> >>>    	u64 flags;
> >>> +	bool do_backup = (max_mirrors == 0);
> >>
> >> Why not replacing @max_mirrors with @do_backup as parameter?
> > 
> > If I read the code correctly, max_mirrors is not just for deciding
> > backup.
> 
> That's strange.
> 
> write_all_supers() only uses @max_mirrors by passing it to 
> write_dev_supers() and wait_dev_supers().
> 
> Both of the write/wait_dev_supers() will replace @max_mirrors to 
> BTRFS_SUPER_MIRROR_MAX if it's zero.
> 
> Further more, all write_all_supers() callers just pass @max_mirrors as 
> bool (either 0 or 1).
> 
> So I don't see any point not replacing the parameter as bool.

Agreed, the idea was to replace the parameter by the bool.

^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-09-24 13:26 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-13 18:25 [PATCH] Btrfs: do not backup tree roots when fsync Liu Bo
2017-09-14  1:55 ` Qu Wenruo
2017-09-14 12:49   ` David Sterba
2017-09-15 21:09     ` Liu Bo
2017-09-22 23:36 ` [PATCH] Btrfs: use self-explaining variable Liu Bo
2017-09-23  0:46   ` Qu Wenruo
2017-09-23  0:48     ` Liu Bo
2017-09-23  1:09       ` Qu Wenruo
2017-09-24 13:24         ` David Sterba

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.