Possible btrfs deadlock coused by commit 660d3f6c

Possible btrfs deadlock coused by commit 660d3f6c

[Stanislaw Gruszka]

Hi Josef,

commit 660d3f6cde552323578b85fc5a09a6742f1fe804
Author: Josef Bacik <josef@redhat.com>
Date:   Fri Dec 9 11:18:51 2011 -0500

    Btrfs: fix how we do delalloc reservations and how we free reservations on error

introduced possible deadlock. According to comment before
btrfs_page_mkwrite(), that function is not allowed to take
node->i_mutex .

The problem was detected by Patrick (full lockdep info is provided):
http://bugzilla.kernel.org/attachment.cgi?id=72072
during investigating of some other issue (kernel.org bug 42576)

Stanislaw

Re: Possible btrfs deadlock coused by commit 660d3f6c

[Josef Bacik]

On Mon, Jan 16, 2012 at 09:53:23AM +0100, Stanislaw Gruszka wrote:
> Hi Josef,
> 
> commit 660d3f6cde552323578b85fc5a09a6742f1fe804
> Author: Josef Bacik <josef@redhat.com>
> Date:   Fri Dec 9 11:18:51 2011 -0500
> 
>     Btrfs: fix how we do delalloc reservations and how we free reservations on error
> 
> introduced possible deadlock. According to comment before
> btrfs_page_mkwrite(), that function is not allowed to take
> node->i_mutex .
> 
> The problem was detected by Patrick (full lockdep info is provided):
> http://bugzilla.kernel.org/attachment.cgi?id=72072
> during investigating of some other issue (kernel.org bug 42576)
> 

It's not a deadlock since you can't mmap a directory, but I posted a fix for
this to linux-btrfs last week, afaik it's headed to -rc1.  Thanks,

Joesf

Re: Possible btrfs deadlock coused by commit 660d3f6c

[Al Viro]

On Mon, Jan 16, 2012 at 09:26:00AM -0500, Josef Bacik wrote:
> On Mon, Jan 16, 2012 at 09:53:23AM +0100, Stanislaw Gruszka wrote:
> > Hi Josef,
> > 
> > commit 660d3f6cde552323578b85fc5a09a6742f1fe804
> > Author: Josef Bacik <josef@redhat.com>
> > Date:   Fri Dec 9 11:18:51 2011 -0500
> > 
> >     Btrfs: fix how we do delalloc reservations and how we free reservations on error
> > 
> > introduced possible deadlock. According to comment before
> > btrfs_page_mkwrite(), that function is not allowed to take
> > node->i_mutex .
> > 
> > The problem was detected by Patrick (full lockdep info is provided):
> > http://bugzilla.kernel.org/attachment.cgi?id=72072
> > during investigating of some other issue (kernel.org bug 42576)
> > 
> 
> It's not a deadlock since you can't mmap a directory, but I posted a fix for
> this to linux-btrfs last week, afaik it's headed to -rc1.  Thanks,

Sigh...  You know, I'm getting really annoyed with the "can't mmap a directory"
bogosity resurfacing every couple of months or so.  No, you can't mmap a
directory.  You also can't bugger a hedgehog, which is about as relevant.

What you *can* do is write(2) on a regular file.  Which will happily grab
->i_mutex (in case of btrfs, that's done in the beginning of
btrfs_file_aio_write()) and eventually get the user pages in (in case of
btrfs, iov_iter_fault_in_readable() from __btrfs_buffered_write() from
btrfs_file_aio_write()).  IOW, you do get pagefaults with ->i_mutex
held.  Which means ->mmap_sem grabbed while holding ->i_mutex.  No
directories involved...

Re: Possible btrfs deadlock coused by commit 660d3f6c

[Josef Bacik]

On Mon, Jan 16, 2012 at 08:45:29PM +0000, Al Viro wrote:
> On Mon, Jan 16, 2012 at 09:26:00AM -0500, Josef Bacik wrote:
> > On Mon, Jan 16, 2012 at 09:53:23AM +0100, Stanislaw Gruszka wrote:
> > > Hi Josef,
> > > 
> > > commit 660d3f6cde552323578b85fc5a09a6742f1fe804
> > > Author: Josef Bacik <josef@redhat.com>
> > > Date:   Fri Dec 9 11:18:51 2011 -0500
> > > 
> > >     Btrfs: fix how we do delalloc reservations and how we free reservations on error
> > > 
> > > introduced possible deadlock. According to comment before
> > > btrfs_page_mkwrite(), that function is not allowed to take
> > > node->i_mutex .
> > > 
> > > The problem was detected by Patrick (full lockdep info is provided):
> > > http://bugzilla.kernel.org/attachment.cgi?id=72072
> > > during investigating of some other issue (kernel.org bug 42576)
> > > 
> > 
> > It's not a deadlock since you can't mmap a directory, but I posted a fix for
> > this to linux-btrfs last week, afaik it's headed to -rc1.  Thanks,
> 
> Sigh...  You know, I'm getting really annoyed with the "can't mmap a directory"
> bogosity resurfacing every couple of months or so.  No, you can't mmap a
> directory.  You also can't bugger a hedgehog, which is about as relevant.
> 
> What you *can* do is write(2) on a regular file.  Which will happily grab
> ->i_mutex (in case of btrfs, that's done in the beginning of
> btrfs_file_aio_write()) and eventually get the user pages in (in case of
> btrfs, iov_iter_fault_in_readable() from __btrfs_buffered_write() from
> btrfs_file_aio_write()).  IOW, you do get pagefaults with ->i_mutex
> held.  Which means ->mmap_sem grabbed while holding ->i_mutex.  No
> directories involved...

Good point, either way it's been fixed.  Thanks,

Josef

Re: Possible btrfs deadlock coused by commit 660d3f6c

[Chris Samuel]

On 17/01/12 09:01, Josef Bacik wrote:

> Good point, either way it's been fixed.  Thanks,

Is it worth pushing that fix into the stable 3.2.x releases ?

cheers,
Chris
-- 
 Chris Samuel  :  http://www.csamuel.org/  :  Melbourne, VIC