RE: [PATCH 1/2] btrfs: Fix out-of-space bug caused by searching commit_root in find_free_dev_extent()

[Zhao Lei <zhaolei@cn.fujitsu.com>]

Hi, Filipe

> > Reason:
> > Btrfs get free disk space by calling find_free_dev_extent() when
> > re-create chunk for write, but current code set search_commit_root
> > flag in searching tree, so disk spaces which was freed from dev_tree
> > but not commited can not get from find_free_dev_extent(), then
> > btrfs report NO_SPACE on above case.
> 
> Yes, and that is correct.
> If you re-use the same physical space within the same transaction,
> write to it and a crash happens before writing the next super block
> (transaction commit), the next time the fs is mounted it will have
> data or metadata corrupted.
> 
> With your change this could happen:
> 
> 1) Delete metadata block group X, which takes physical space [2G, 3G]
> (assuming single metadata profile);
> 
> 2) A chunk allocation for a data block group happens, and it gets that
> same physical space in the range [2G, 3G];
> 
> 3) Data is written into it - for e.g. due to memory pressure, the VM
> calls writepages() for some inodes;
> 
> 4) Crash/reboot happens before the transaction commits;
> 
> 5) On the next mount, you have metadata corrupted - there's now file
> data instead of btree nodes/leafs in metadata the block group at the
> physical range [2G, 3G]
> 
Thanks for notice.

Since above NO_SPACE bug is still exist and need to be fixed, maybe we
need to do a commit transaction after remove_bgs.
I'll test this way and resend.

Thanks
Zhaolei

> That said, unless I missed something, I disagree with this patch.
> 
> >
> > Fix:
> > Remove "path->search_commit_root = 1" in find_free_dev_extent().
> >
> > Tested by above script, and confirmed action with many printk.
> >
> > Reported-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
> >
> > Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
> > ---
> >  fs/btrfs/volumes.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index 50c5a87..5cd0930 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -1147,7 +1147,6 @@ again:
> >         }
> >
> >         path->reada = 2;
> > -       path->search_commit_root = 1;
> >         path->skip_locking = 1;
> >
> >         key.objectid = device->devid;
> > --
> > 1.8.5.1
> >
> > --
> > 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
> 
> 
> 
> --
> Filipe David Manana,
> 
> "Reasonable men adapt themselves to the world.
>  Unreasonable men adapt the world to themselves.
>  That's why all progress depends on unreasonable men."