On Wed, Jun 17, 2015 at 11:04 AM, Filipe David Manana
<fdmanana@xxxxxxxxx> wrote:
> On Mon, Jun 15, 2015 at 2:41 PM, <jeffm@xxxxxxxx> wrote:
>> From: Jeff Mahoney <jeffm@xxxxxxxx>
>>
>> The cleaner thread may already be sleeping by the time we enter
>> close_ctree. If that's the case, we'll skip removing any unused
>> block groups queued for removal, even during a normal umount.
>> They'll be cleaned up automatically at next mount, but users
>> expect a umount to be a clean synchronization point, especially
>> when used on thin-provisioned storage with -odiscard. We also
>> explicitly remove unused block groups in the ro-remount path
>> for the same reason.
>>
>> Signed-off-by: Jeff Mahoney <jeffm@xxxxxxxx>
> Reviewed-by: Filipe Manana <fdmanana@xxxxxxxx>
> Tested-by: Filipe Manana <fdmanana@xxxxxxxx>
>
>> ---
>> fs/btrfs/disk-io.c | 9 +++++++++
>> fs/btrfs/super.c | 11 +++++++++++
>> 2 files changed, 20 insertions(+)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 2ef9a4b..2e47fef 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -3710,6 +3710,15 @@ void close_ctree(struct btrfs_root *root)
>> cancel_work_sync(&fs_info->async_reclaim_work);
>>
>> if (!(fs_info->sb->s_flags & MS_RDONLY)) {
>> + /*
>> + * If the cleaner thread is stopped and there are
>> + * block groups queued for removal, the deletion will be
>> + * skipped when we quit the cleaner thread.
>> + */
>> + mutex_lock(&root->fs_info->cleaner_mutex);
>> + btrfs_delete_unused_bgs(root->fs_info);
>> + mutex_unlock(&root->fs_info->cleaner_mutex);
>> +
>> ret = btrfs_commit_super(root);
>> if (ret)
>> btrfs_err(fs_info, "commit super ret %d", ret);
>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>> index 9e66f5e..2ccd8d4 100644
>> --- a/fs/btrfs/super.c
>> +++ b/fs/btrfs/super.c
>> @@ -1539,6 +1539,17 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
>>
>> sb->s_flags |= MS_RDONLY;
>>
>> + /*
>> + * Setting MS_RDONLY will put the cleaner thread to
>> + * sleep at the next loop if it's already active.
>> + * If it's already asleep, we'll leave unused block
>> + * groups on disk until we're mounted read-write again
>> + * unless we clean them up here.
>> + */
>> + mutex_lock(&root->fs_info->cleaner_mutex);
>> + btrfs_delete_unused_bgs(fs_info);
>> + mutex_unlock(&root->fs_info->cleaner_mutex);
So actually, this allows for a deadlock after the patch I sent out last week:
https://patchwork.kernel.org/patch/6586811/
In that patch delete_unused_bgs is no longer called under the
cleaner_mutex, and making it so, will cause a deadlock with
relocation.
Even without that patch, I don't think you need using this mutex
anyway - no 2 tasks running this function can get the same bg from the
fs_info->unused_bgs list.
thanks
>> +
>> btrfs_dev_replace_suspend_for_unmount(fs_info);
>> btrfs_scrub_cancel(fs_info);
>> btrfs_pause_balance(fs_info);
>> --
>> 2.4.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
