Re: [PATCH 2/4] Btrfs: fix deadlock on sb->s_umount when doing umount

[Miao Xie <miaox@cn.fujitsu.com>]

On Mon, 30 Apr 2012 18:41:39 +0200, David Sterba wrote:
> On Fri, Apr 27, 2012 at 06:55:13PM +0800, Miao Xie wrote:
>> But I found you add a trylock for ->s_umount in cleaner_kthread(), this method
>> can fix the deadlock problem, I think. It may be introduced by the other patch,
>> could you send that patch to me? I found if we fail to trylock ->cleaner_mutex,
>> we will forget to unlock ->s_umount.
> 
> the unlock was not visible within the diff context, the full patch is:
> 
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -1578,11 +1578,13 @@ static int cleaner_kthread(void *arg)
>                 vfs_check_frozen(root->fs_info->sb, SB_FREEZE_WRITE);
> 
>                 if (!(root->fs_info->sb->s_flags & MS_RDONLY) &&
> +                       down_read_trylock(&root->fs_info->sb->s_umount) &&
>                     mutex_trylock(&root->fs_info->cleaner_mutex)) {
>                         btrfs_run_delayed_iputs(root);
>                         btrfs_clean_old_snapshots(root);
>                         mutex_unlock(&root->fs_info->cleaner_mutex);
>                         btrfs_run_defrag_inodes(root->fs_info);
> +                       up_read(&root->fs_info->sb->s_umount);
>                 }
> 
>                 if (freezing(current)) {
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -3413,7 +3413,9 @@ static int shrink_delalloc(struct btrfs_
>                          max_reclaim >> PAGE_CACHE_SHIFT);
>         while (loops < 1024) {
>                 /* have the flusher threads jump in and do some IO */
> -               smp_mb();
> +               if (btrfs_fs_closing(root->fs_info))
> +                       return -EAGAIN;
> +
>                 nr_pages = min_t(unsigned long, nr_pages,
>                        root->fs_info->delalloc_bytes >> PAGE_CACHE_SHIFT);
>                 writeback_inodes_sb_nr_if_idle(root->fs_info->sb, nr_pages);
> ---
> 
> after close_tree starts the "fs_closing" check will prevent further
> calls to writeback. I don't remember from who the patch acutally comes
> from (via irc), it was noted as a workaround for the cleaner deadlock
> until it gets fully solved (re your patches
> http://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg13897.html
> and reference to balance and scrub:
> http://www.spinics.net/lists/linux-btrfs/msg14056.html
> )

Sorry, I forget to reply this mail.

I think this method can not fix the problem safely because if the other
background threads(not the cleaner) call shrink_delalloc(), the problem
can still occur.

Though trylock for the cleaner can not fix this problem, I think the cleaner
still need trylock ->s_umount, in this way, we can stop the cleaner making
lots of dirty pages when we do readonly remount or umount.

Thanks
Miao