[PATCH 1/4] Btrfs: fix hang on snapshot creation after RWF_NOWAIT write

[PATCH 1/4] Btrfs: fix hang on snapshot creation after RWF_NOWAIT write

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

If we do a successful RWF_NOWAIT write we end up locking the snapshot lock
of the inode, through a call to check_can_nocow(), but we never unlock it.

This means the next attempt to create a snapshot on the subvolume will
hang forever.

Trivial reproducer:

  $ mkfs.btrfs -f /dev/sdb
  $ mount /dev/sdb /mnt

  $ touch /mnt/foobar
  $ chattr +C /mnt/foobar
  $ xfs_io -d -c "pwrite -S 0xab 0 64K" /mnt/foobar
  $ xfs_io -d -c "pwrite -N -V 1 -S 0xfe 0 64K" /mnt/foobar

  $ btrfs subvolume snapshot -r /mnt /mnt/snap
    --> hangs

Fix this by unlocking the snapshot lock if check_can_nocow() returned
success.

Fixes: edf064e7c6fec3 ("btrfs: nowait aio support")
CC: stable@vger.kernel.org # 4.13+
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/file.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 2c14312b05e8..04faa04fccd1 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1914,6 +1914,8 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
 			inode_unlock(inode);
 			return -EAGAIN;
 		}
+		/* check_can_nocow() locks the snapshot lock on success */
+		btrfs_drew_write_unlock(&root->snapshot_lock);
 	}
 
 	current->backing_dev_info = inode_to_bdi(inode);
-- 
2.26.2

Re: [PATCH 1/4] Btrfs: fix hang on snapshot creation after RWF_NOWAIT write

[David Sterba]

On Mon, Jun 15, 2020 at 06:46:01PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> If we do a successful RWF_NOWAIT write we end up locking the snapshot lock
> of the inode, through a call to check_can_nocow(), but we never unlock it.
> 
> This means the next attempt to create a snapshot on the subvolume will
> hang forever.
> 
> Trivial reproducer:
> 
>   $ mkfs.btrfs -f /dev/sdb
>   $ mount /dev/sdb /mnt
> 
>   $ touch /mnt/foobar
>   $ chattr +C /mnt/foobar
>   $ xfs_io -d -c "pwrite -S 0xab 0 64K" /mnt/foobar
>   $ xfs_io -d -c "pwrite -N -V 1 -S 0xfe 0 64K" /mnt/foobar
> 
>   $ btrfs subvolume snapshot -r /mnt /mnt/snap
>     --> hangs
> 
> Fix this by unlocking the snapshot lock if check_can_nocow() returned
> success.
> 
> Fixes: edf064e7c6fec3 ("btrfs: nowait aio support")
> CC: stable@vger.kernel.org # 4.13+
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>  fs/btrfs/file.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 2c14312b05e8..04faa04fccd1 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1914,6 +1914,8 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
>  			inode_unlock(inode);
>  			return -EAGAIN;
>  		}
> +		/* check_can_nocow() locks the snapshot lock on success */
> +		btrfs_drew_write_unlock(&root->snapshot_lock);

That's quite ugly that the locking semantics of check_can_nocow is
hidden, this should be cleaned up too.

The whole condition

1909                 if (!(BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW |
1910                                               BTRFS_INODE_PREALLOC)) ||
1911                     check_can_nocow(BTRFS_I(inode), pos, &count) <= 0)

has 2 parts and it's not obvious from the context when the lock actually is
taken. The flags check could be pushed down to check_can_nocow, the
same but negated condition can be found in btrfs_file_write_iter so this
would make it something like:

	if (check_can_nocow(inode, pos, &count) <= 0) {
		/* fallback */
		return ...;
	}
	/*
	 * the lock is taken and needs to be unlocked at the right time
	 */

Suggestions to rename check_can_nocow welcome too.


>  	}
>  
>  	current->backing_dev_info = inode_to_bdi(inode);
> -- 
> 2.26.2

Re: [PATCH 1/4] Btrfs: fix hang on snapshot creation after RWF_NOWAIT write

[Filipe Manana]

On Tue, Jun 16, 2020 at 3:34 PM David Sterba <dsterba@suse.cz> wrote:
>
> On Mon, Jun 15, 2020 at 06:46:01PM +0100, fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > If we do a successful RWF_NOWAIT write we end up locking the snapshot lock
> > of the inode, through a call to check_can_nocow(), but we never unlock it.
> >
> > This means the next attempt to create a snapshot on the subvolume will
> > hang forever.
> >
> > Trivial reproducer:
> >
> >   $ mkfs.btrfs -f /dev/sdb
> >   $ mount /dev/sdb /mnt
> >
> >   $ touch /mnt/foobar
> >   $ chattr +C /mnt/foobar
> >   $ xfs_io -d -c "pwrite -S 0xab 0 64K" /mnt/foobar
> >   $ xfs_io -d -c "pwrite -N -V 1 -S 0xfe 0 64K" /mnt/foobar
> >
> >   $ btrfs subvolume snapshot -r /mnt /mnt/snap
> >     --> hangs
> >
> > Fix this by unlocking the snapshot lock if check_can_nocow() returned
> > success.
> >
> > Fixes: edf064e7c6fec3 ("btrfs: nowait aio support")
> > CC: stable@vger.kernel.org # 4.13+
> > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > ---
> >  fs/btrfs/file.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> > index 2c14312b05e8..04faa04fccd1 100644
> > --- a/fs/btrfs/file.c
> > +++ b/fs/btrfs/file.c
> > @@ -1914,6 +1914,8 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
> >                       inode_unlock(inode);
> >                       return -EAGAIN;
> >               }
> > +             /* check_can_nocow() locks the snapshot lock on success */
> > +             btrfs_drew_write_unlock(&root->snapshot_lock);
>
> That's quite ugly that the locking semantics of check_can_nocow is
> hidden, this should be cleaned up too.
>
> The whole condition
>
> 1909                 if (!(BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW |
> 1910                                               BTRFS_INODE_PREALLOC)) ||
> 1911                     check_can_nocow(BTRFS_I(inode), pos, &count) <= 0)
>
> has 2 parts and it's not obvious from the context when the lock actually is
> taken. The flags check could be pushed down to check_can_nocow, the
> same but negated condition can be found in btrfs_file_write_iter so this
> would make it something like:
>
>         if (check_can_nocow(inode, pos, &count) <= 0) {
>                 /* fallback */
>                 return ...;
>         }
>         /*
>          * the lock is taken and needs to be unlocked at the right time
>          */
>
> Suggestions to rename check_can_nocow welcome too.

Sure, I can understand it may look not obvious on first sight at least.

Here I'm only focusing on functional problems and kept this fix as
small as possible to backport to stable releases,
as this is a bug that directly impacts user experience.

Thanks.

>
>
> >       }
> >
> >       current->backing_dev_info = inode_to_bdi(inode);
> > --
> > 2.26.2

Re: [PATCH 1/4] Btrfs: fix hang on snapshot creation after RWF_NOWAIT write

[David Sterba]

On Tue, Jun 16, 2020 at 04:17:19PM +0100, Filipe Manana wrote:
> On Tue, Jun 16, 2020 at 3:34 PM David Sterba <dsterba@suse.cz> wrote:
> > On Mon, Jun 15, 2020 at 06:46:01PM +0100, fdmanana@kernel.org wrote:
> > > From: Filipe Manana <fdmanana@suse.com>
> > > --- a/fs/btrfs/file.c
> > > +++ b/fs/btrfs/file.c
> > > @@ -1914,6 +1914,8 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
> > >                       inode_unlock(inode);
> > >                       return -EAGAIN;
> > >               }
> > > +             /* check_can_nocow() locks the snapshot lock on success */
> > > +             btrfs_drew_write_unlock(&root->snapshot_lock);
> >
> > That's quite ugly that the locking semantics of check_can_nocow is
> > hidden, this should be cleaned up too.
> >
> > The whole condition
> >
> > 1909                 if (!(BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW |
> > 1910                                               BTRFS_INODE_PREALLOC)) ||
> > 1911                     check_can_nocow(BTRFS_I(inode), pos, &count) <= 0)
> >
> > has 2 parts and it's not obvious from the context when the lock actually is
> > taken. The flags check could be pushed down to check_can_nocow, the
> > same but negated condition can be found in btrfs_file_write_iter so this
> > would make it something like:
> >
> >         if (check_can_nocow(inode, pos, &count) <= 0) {
> >                 /* fallback */
> >                 return ...;
> >         }
> >         /*
> >          * the lock is taken and needs to be unlocked at the right time
> >          */
> >
> > Suggestions to rename check_can_nocow welcome too.
> 
> Sure, I can understand it may look not obvious on first sight at least.
> 
> Here I'm only focusing on functional problems and kept this fix as
> small as possible to backport to stable releases,
> as this is a bug that directly impacts user experience.

Ok that makes sense of course, I'll add the four patches to misc-next
and queue them for rc. Thanks.

Re: [PATCH 1/4] Btrfs: fix hang on snapshot creation after RWF_NOWAIT write

[Josef Bacik]

On 6/15/20 1:46 PM, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> If we do a successful RWF_NOWAIT write we end up locking the snapshot lock
> of the inode, through a call to check_can_nocow(), but we never unlock it.
> 
> This means the next attempt to create a snapshot on the subvolume will
> hang forever.
> 
> Trivial reproducer:
> 
>    $ mkfs.btrfs -f /dev/sdb
>    $ mount /dev/sdb /mnt
> 
>    $ touch /mnt/foobar
>    $ chattr +C /mnt/foobar
>    $ xfs_io -d -c "pwrite -S 0xab 0 64K" /mnt/foobar
>    $ xfs_io -d -c "pwrite -N -V 1 -S 0xfe 0 64K" /mnt/foobar
> 
>    $ btrfs subvolume snapshot -r /mnt /mnt/snap
>      --> hangs
> 
> Fix this by unlocking the snapshot lock if check_can_nocow() returned
> success.
> 
> Fixes: edf064e7c6fec3 ("btrfs: nowait aio support")
> CC: stable@vger.kernel.org # 4.13+
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef