On Mon, Nov 16, 2015 at 10:07 AM, Zhao Lei <zhao...@cn.fujitsu.com> wrote:
> Hi, Filipe Manana
>
> Thanks for review.
>
>> -----Original Message-----
>> From: Filipe Manana [mailto:fdman...@gmail.com]
>> Sent: Friday, November 13, 2015 8:02 PM
>> To: Zhao Lei <zhao...@cn.fujitsu.com>
>> Cc: linux-btrfs@vger.kernel.org
>> Subject: Re: [PATCH] btrfs: Continue replace when set_block_ro failed
>>
>> On Fri, Nov 13, 2015 at 11:38 AM, Zhao Lei <zhao...@cn.fujitsu.com> wrote:
>> > xfstests/011 failed in node with small_size filesystem.
>> > Can be reproduced by following script:
>> > DEV_LIST="/dev/vdd /dev/vde"
>> > DEV_REPLACE="/dev/vdf"
>> >
>> > do_test()
>> > {
>> > local mkfs_opt="$1"
>> > local size="$2"
>> >
>> > dmesg -c >/dev/null
>> > umount $SCRATCH_MNT &>/dev/null
>> >
>> > echo mkfs.btrfs -f $mkfs_opt "${DEV_LIST[*]}"
>> > mkfs.btrfs -f $mkfs_opt "${DEV_LIST[@]}" || return 1
>> > mount "${DEV_LIST[0]}" $SCRATCH_MNT
>> >
>> > echo -n "Writing big files"
>> > dd if=/dev/urandom of=$SCRATCH_MNT/t0 bs=1M
>> count=1 >/dev/null 2>&1
>> > for ((i = 1; i <= size; i++)); do
>> > echo -n .
>> > /bin/cp $SCRATCH_MNT/t0 $SCRATCH_MNT/t$i || return 1
>> > done
>> > echo
>> >
>> > echo Start replace
>> > btrfs replace start -Bf "${DEV_LIST[0]}" "$DEV_REPLACE"
>> $SCRATCH_MNT || {
>> > dmesg
>> > return 1
>> > }
>> > return 0
>> > }
>> >
>> > # Set size to value near fs size
>> > # for example, 1897 can trigger this bug in 2.6G device.
>> > #
>> > ./do_test "-d raid1 -m raid1" 1897
>> >
>> > System will report replace fail with following warning in dmesg:
>> >
>> > Reason:
>> > When big data writen to fs, the whole free space will be allocated
>> > for data chunk.
>> > And operation as scrub need to set_block_ro(), and when there is
>> > only one metadata chunk in system(or other metadata chunks are all
>> > full), the function will try to allocate a new chunk, and failed
>> > because no space in device.
>> >
>> > Fix:
>> > When set_block_ro failed for metadata chunk, it is not a problem
>> > because scrub_lock forbids commit_trancaction.
>>
>> Hi Zhao, I'm confused reading this explanation.
>>
>> Why isn't it a problem if the block group gets modified while the
>> transaction is
>> ongoing? Through writepages() for example.
>> Committing a transaction isn't the only way to write data or metadata to a
>> block group.
>>
> Metadata chunks always updated in cow, the writepages() will write
> new data to different place with operation done by replace.
So why do we try to set the block group ro? Your change is really
confusing - if we fail setting the block group to read-only mode, we
just ignore it and proceed - why bother in the first place to set it
read-only? Someone reading the code will find it fishy, and going back
to git history, your change log doesn't really explains why this is
being done and why is it safe to do it.
You also forgot to paste the warning you mention below "System will
report replace fail with following warning in dmesg:" in the change
log.
thanks
>
> Thanks
> Zhaolei
>
>> thanks
>>
>> > Let replace continue in this case is no problem.
>> >
>> > Tested by above script, and xfstests/011, plus 100 times xfstests/070.
>> >
>> > Signed-off-by: Zhao Lei <zhao...@cn.fujitsu.com>
>> > ---
>> > fs/btrfs/scrub.c | 9 ++++-----
>> > 1 file changed, 4 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c index
>> > a39f5d1..5a30753 100644
>> > --- a/fs/btrfs/scrub.c
>> > +++ b/fs/btrfs/scrub.c
>> > @@ -3532,6 +3532,7 @@ int scrub_enumerate_chunks(struct scrub_ctx
>> *sctx,
>> > u64 length;
>> > u64 chunk_offset;
>> > int ret = 0;
>> > + int ro_set = 0;
>> > int slot;
>> > struct extent_buffer *l;
>> > struct btrfs_key key;
>> > @@ -3617,10 +3618,7 @@ int scrub_enumerate_chunks(struct scrub_ctx
>> *sctx,
>> > scrub_pause_on(fs_info);
>> > ret = btrfs_inc_block_group_ro(root, cache);
>> > scrub_pause_off(fs_info);
>> > - if (ret) {
>> > - btrfs_put_block_group(cache);
>> > - break;
>> > - }
>> > + ro_set = !ret;
>> >
>> > dev_replace->cursor_right = found_key.offset + length;
>> > dev_replace->cursor_left = found_key.offset; @@
>> > -3660,7 +3658,8 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx,
>> >
>> > scrub_pause_off(fs_info);
>> >
>> > - btrfs_dec_block_group_ro(root, cache);
>> > + if (ro_set)
>> > + btrfs_dec_block_group_ro(root, cache);
>> >
>> > btrfs_put_block_group(cache);
>> > if (ret)
>> > --
>> > 1.8.5.1
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
>> > in the body of a message to majord...@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."
>
--
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."
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
