On Mon, Feb 24, 2014 at 3:44 PM, Austin S Hemmelgarn <[email protected]> wrote: > On 2014-02-24 08:37, Ilya Dryomov wrote: >> On Thu, Feb 20, 2014 at 6:57 PM, David Sterba <[email protected]> wrote: >>> On Wed, Feb 19, 2014 at 11:10:41AM -0500, Austin S Hemmelgarn wrote: >>>> Currently, btrfs balance start fails when trying to convert metadata or >>>> system chunks to dup profile on filesystems with multiple devices. This >>>> requires that a conversion from a multi-device filesystem to a single >>>> device filesystem use the following methodology: >>>> 1. btrfs balance start -dconvert=single -mconvert=single \ >>>> -sconvert=single -f / >>>> 2. btrfs device delete / /dev/sdx >>>> 3. btrfs balance start -mconvert=dup -sconvert=dup / >>>> This results in a period of time (possibly very long if the devices are >>>> big) where you don't have the protection guarantees of multiple copies >>>> of metadata chunks. >>>> >>>> After applying this patch, one can instead use the following methodology >>>> for conversion from a multi-device filesystem to a single device >>>> filesystem: >>>> 1. btrfs balance start -dconvert=single -mconvert=dup \ >>>> -sconvert=dup -f / >>>> 2. btrfs device delete / /dev/sdx >>>> This greatly reduces the chances of the operation causing data loss due >>>> to a read error during the device delete. >>>> >>>> Signed-off-by: Austin S. Hemmelgarn <[email protected]> >>> Reviewed-by: David Sterba <[email protected]> >>> >>> Sounds useful. The muliple devices + DUP is allowed setup when the >>> device is added, this patch only adds the 'delete' counterpart. The >>> imroved data loss protection during the process is a good thing. >> >> Hi, >> >> Have you actually tried to queue it? Unless I'm missing something, it won't >> compile, and on top of that, it seems to be corrupted too.. > The patch itself was made using git, AFAICT it should be fine. I've > personally built and tested it using UML.
It doesn't look fine. It was generated with git, but it got corrupted on the way: either how you pasted it or the email client you use is the problem. On Wed, Feb 19, 2014 at 6:10 PM, Austin S Hemmelgarn <[email protected]> wrote: > Currently, btrfs balance start fails when trying to convert metadata or > system chunks to dup profile on filesystems with multiple devices. This > requires that a conversion from a multi-device filesystem to a single > device filesystem use the following methodology: > 1. btrfs balance start -dconvert=single -mconvert=single \ > -sconvert=single -f / > 2. btrfs device delete / /dev/sdx > 3. btrfs balance start -mconvert=dup -sconvert=dup / > This results in a period of time (possibly very long if the devices are > big) where you don't have the protection guarantees of multiple copies > of metadata chunks. > > After applying this patch, one can instead use the following methodology > for conversion from a multi-device filesystem to a single device > filesystem: > 1. btrfs balance start -dconvert=single -mconvert=dup \ > -sconvert=dup -f / > 2. btrfs device delete / /dev/sdx > This greatly reduces the chances of the operation causing data loss due > to a read error during the device delete. > > Signed-off-by: Austin S. Hemmelgarn <[email protected]> > --- > fs/btrfs/volumes.c | 21 +++++++++++++++++---- > 1 file changed, 17 insertions(+), 4 deletions(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 07629e9..38a9522 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -3152,10 +3152,8 @@ int btrfs_balance(struct btrfs_balance_control > *bctl, ^^^, that should be a single line > num_devices--; > } > btrfs_dev_replace_unlock(&fs_info->dev_replace); > - allowed = BTRFS_AVAIL_ALLOC_BIT_SINGLE; > - if (num_devices == 1) > - allowed |= BTRFS_BLOCK_GROUP_DUP; > - else if (num_devices > 1) > + allowed = BTRFS_AVAIL_ALLOC_BIT_SINGLE | BTRFS_BLOCK_GROUP_DUP; > + if (num_devices > 1) > allowed |= (BTRFS_BLOCK_GROUP_RAID0 | > BTRFS_BLOCK_GROUP_RAID1); > if (num_devices > 2) > allowed |= BTRFS_BLOCK_GROUP_RAID5; > @@ -3221,6 +3219,21 @@ int btrfs_balance(struct btrfs_balance_control > *bctl, ^^^, ditto > goto out; > } > } > + if (((bctl->sys.flags & BTRFS_BALANCE_ARGS_CONVERT) && > + (bctl->sys.target & ~BTRFS_BLOCK_GROUP_DUP) || > + (bctl->meta.flags & BTRFS_BALANCE_ARGS_CONVERT) && > + (bctl->meta.target & ~BTRFS_BLOCK_GROUP_DUP)) && > + (num_devs > 1)) { > + if (bctl->flags & BTRFS_BALANCE_FORCE) { > + btrfs_info(fs_info, "force conversion of > metadata " > + "to dup profile on multiple > devices"); > + } else { > + btrfs_err(fs_info, "balance will reduce > metadata " > + "integrity, use force if you want > this"); > + ret = -EINVAL; > + goto out; > + } > + } > } while (read_seqretry(&fs_info->profiles_lock, seq)); > if (bctl->sys.flags & BTRFS_BALANCE_ARGS_CONVERT) { ^^^, there should be 3 lines of context here. It looks like an empty line between "} while ..." and "if (bctl..." is missing. There is probably more, those just stand out. Thanks, Ilya -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
