[v2,0/3] btrfs: no longer hold the extent lock for the entire read

Message

Josef Bacik Aug. 26, 2024, 4:37 p.m. UTC

v1: https://lore.kernel.org/linux-btrfs/[e-mail]/

v1->v2:
- Added Goldwyn's bit to move the extent lock to only cover the part where we
  look up the extent map, added his Co-developed-by to the patch.

-- Original email --
Hello,

One of the biggest obstacles to switching to iomap is that our extent locking is
a bit wonky.  We've made a lot of progress on the write side to drastically
narrow the scope of the extent lock, but the read side is arguably the most
problematic in that we hold it until the readpage is completed.

This patch series addresses this by no longer holding the lock for the entire
IO.  This is safe on the buffered side because we are protected by the page lock
and the checks for ordered extents when we start the write.

The direct io side is the more problematic side, since we could now end up with
overlapping and concurrent direct writes in direct read ranges.  To solve this
problem I'm introducing a new extent io bit to do range locking for direct IO.
This will work basically the same as the extent lock did before, but only for
direct IO.  We are saved by the normal inode lock and page cache in the mixed
buffered and direct case, so this shouldn't carry the same iomap+locking
reloated woes that the extent lock did.

This will also allow us to remove the page fault restrictions in the direct IO
case, which will be done in a followup series.

I've run this through the CI and a lot of local testing, I'm keeping it small
and targeted because this is a pretty big logical shift for us, so I want to
make sure we get good testing on it before I go doing the other larger projects.
Thanks,

Josef

Josef Bacik (3):
  btrfs: introduce EXTENT_DIO_LOCKED
  btrfs: take the dio extent lock during O_DIRECT operations
  btrfs: do not hold the extent lock for entire read

 fs/btrfs/compression.c    |  2 +-
 fs/btrfs/direct-io.c      | 72 +++++++++++++++++++-----------
 fs/btrfs/extent-io-tree.c | 52 ++++++++++------------
 fs/btrfs/extent-io-tree.h | 38 ++++++++++++++--
 fs/btrfs/extent_io.c      | 94 ++-------------------------------------
 5 files changed, 109 insertions(+), 149 deletions(-)

Comments

David Sterba Aug. 27, 2024, 12:53 a.m. UTC | #1

On Mon, Aug 26, 2024 at 12:37:23PM -0400, Josef Bacik wrote:
> v1: https://lore.kernel.org/linux-btrfs/[e-mail]/
> 
> v1->v2:
> - Added Goldwyn's bit to move the extent lock to only cover the part where we
>   look up the extent map, added his Co-developed-by to the patch.
> 
> -- Original email --
> Hello,
> 
> One of the biggest obstacles to switching to iomap is that our extent locking is
> a bit wonky.  We've made a lot of progress on the write side to drastically
> narrow the scope of the extent lock, but the read side is arguably the most
> problematic in that we hold it until the readpage is completed.
> 
> This patch series addresses this by no longer holding the lock for the entire
> IO.  This is safe on the buffered side because we are protected by the page lock
> and the checks for ordered extents when we start the write.
> 
> The direct io side is the more problematic side, since we could now end up with
> overlapping and concurrent direct writes in direct read ranges.  To solve this
> problem I'm introducing a new extent io bit to do range locking for direct IO.
> This will work basically the same as the extent lock did before, but only for
> direct IO.  We are saved by the normal inode lock and page cache in the mixed
> buffered and direct case, so this shouldn't carry the same iomap+locking
> reloated woes that the extent lock did.
> 
> This will also allow us to remove the page fault restrictions in the direct IO
> case, which will be done in a followup series.
> 
> I've run this through the CI and a lot of local testing, I'm keeping it small
> and targeted because this is a pretty big logical shift for us, so I want to
> make sure we get good testing on it before I go doing the other larger projects.
> Thanks,

We don't have anything that would interfere with that in for-next, the
folio conversions are mostly direct and there were no new problems
preported. The read locking change will probably lead to some
performance gain so I guess this is a good improvement for the next
release.
> 
> Josef
> 
> Josef Bacik (3):
>   btrfs: introduce EXTENT_DIO_LOCKED
>   btrfs: take the dio extent lock during O_DIRECT operations

Patches 1 and 2 are easy, so

Reviewed-by: David Sterba <[e-mail]>

>   btrfs: do not hold the extent lock for entire read

I don't see anything obviously wrong here, the logic change makes sense
and code follows that.