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@xxxxxxx> wrote:
> > On Mon, Jun 15, 2020 at 06:46:01PM +0100, fdmanana@xxxxxxxxxx wrote:
> > > From: Filipe Manana <fdmanana@xxxxxxxx>
> > > --- 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.
