Re: [PATCH] Btrfs: Fix deadlock between direct IO and fast fsync

[Chandan Rajendra <chandan@linux.vnet.ibm.com>]

On Friday, December 23, 2016 04:18:00 PM Liu Bo wrote:
> On Fri, Dec 23, 2016 at 05:27:55PM +0530, Chandan Rajendra wrote:
> > On Friday, December 23, 2016 03:57:40 PM Chandan Rajendra wrote:
> > > On Friday, December 23, 2016 03:00:18 PM Chandan Rajendra wrote:
> > > > The following deadlock is seen when executing generic/113 test,
> > > > 
> > > >  ---------------------------------------------------------+----------------------------------------------------
> > > >   Direct I/O task                                           Fast fsync task
> > > >  ---------------------------------------------------------+----------------------------------------------------
> > > >   btrfs_direct_IO
> > > >     __blockdev_direct_IO
> > > >      do_blockdev_direct_IO
> > > >       do_direct_IO
> > > >        btrfs_get_blocks_direct
> > > >         while (blocks needs to written)
> > > >          get_more_blocks (first iteration)
> > > >           btrfs_get_blocks_direct
> > > >            btrfs_create_dio_extent
> > > >              down_read(&BTRFS_I(inode) >dio_sem)
> > > >              Create and add extent map and ordered extent
> > > >              up_read(&BTRFS_I(inode) >dio_sem)
> > > >                                                             btrfs_sync_file
> > > >                                                               btrfs_log_dentry_safe
> > > >                                                                btrfs_log_inode_parent
> > > >                                                                 btrfs_log_inode
> > > >                                                                  btrfs_log_changed_extents
> > > >                                                                   down_write(&BTRFS_I(inode) >dio_sem)
> > > >                                                                    Collect new extent maps and ordered extents
> > > >                                                                     wait for ordered extent completion
> > > >          get_more_blocks (second iteration)
> > > >           btrfs_get_blocks_direct
> > > >            btrfs_create_dio_extent
> > > >              down_read(&BTRFS_I(inode) >dio_sem)
> > > >  --------------------------------------------------------------------------------------------------------------
> > > > 
> > > > In the above description, Btrfs direct I/O code path has not yet started
> > > > submitting bios for file range covered by the initial ordered
> > > > extent. Meanwhile, The fast fsync task obtains the write semaphore and
> > > > waits for I/O on the ordered extent to get completed. However, the
> > > > Direct I/O task is now blocked on obtaining the read semaphore.
> > > > 
> > > > To resolve the deadlock, this commit modifies the Direct I/O code path
> > > > to obtain the read semaphore before invoking
> > > > __blockdev_direct_IO(). The semaphore is then given up after
> > > > __blockdev_direct_IO() returns. This allows the Direct I/O code to
> > > > complete I/O on all the ordered extents it creates.
> > > >
> > > 
> > > Btw, I was able to reproduce the issue on kdave/for-next branch with "Merge
> > > branch 'for-next-next-4.9-20161125' into for-next-20161125" as the topmost
> > > commit. The issue cannot be reproduced yet on latest code available from
> > > kdave/for-next branch.
> > > 
> > >
> > 
> > Maybe changes in upstream might have masked the issue in the recent
> > kdave/for-next branch. I say that because 'git bisect' resulted in the
> > following commit ...
> 
> I guess that the for-next branch didn't revert this patch[1] as upstream
> did, so that generic/113 would complain, however, even w/o that patch,
> this fix is still required since the deadlock could be reproduced by
> running generic/113 with '-ofragment=data' and in fact Filipe has
> proposed a almost same fix but not a real patch in this thread [2].
> 
> [1]: Btrfs: adjust len of writes if following a preallocated extent
> https://patchwork.kernel.org/patch/9413129/
> [2]: https://patchwork.kernel.org/patch/9445231/
>

Ah ok. Thanks for pointing it out.

-- 
chandan