On Tue, Jun 16, 2020 at 3:34 PM David Sterba <dsterba@xxxxxxx> wrote:
>
> On Mon, Jun 15, 2020 at 06:46:01PM +0100, fdmanana@xxxxxxxxxx wrote:
> > From: Filipe Manana <fdmanana@xxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx # 4.13+
> > Signed-off-by: Filipe Manana <fdmanana@xxxxxxxx>
> > ---
> > 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
