| Message ID | cover.1724095925.git.rgoldwyn@suse.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Reduce scope of extent locking while reading | expand |
On Mon, Aug 19, 2024 at 04:00:51PM -0400, Goldwyn Rodrigues wrote: > From: Goldwyn Rodrigues <rgoldwyn@suse.com> > > Extent locking is not required for the entire read process. Once the > extent map is read all information to retrieve is available in the > extent map, and is cached in em_cached for later retrieval. The extent > map cached is also refcounted so it would not disappear. > Limit the extent locking while reading the extnet maps only. The rest is > just creating bio and fetching/decompressing the data according to the > extent map provided. > > The only reason this was not working is because we would get EIO as > the CRCs would not match (or were not committed to disk as yet) for > folios that were written and released. In order to get over it, mark the > extent as finished only after all folios are cleared of writeback. > > I have run the xfstests on it without any deadlocks or corruptions. > However, a fresh outlook on what could go wrong with a limiting the > scope of locks would be good. > > Goldwyn Rodrigues (2): > btrfs: clear folio writeback after completing ordered range > btrfs: take extent lock only while reading extent map The first patch seems relevant but it had been sent before Josef added the read locking updates so I don't know if it still needs to be merged. The second patch is covered by "btrfs: do not hold the extent lock for entire read".
On 19:53 29/08, David Sterba wrote: > On Mon, Aug 19, 2024 at 04:00:51PM -0400, Goldwyn Rodrigues wrote: > > From: Goldwyn Rodrigues <rgoldwyn@suse.com> > > > > Extent locking is not required for the entire read process. Once the > > extent map is read all information to retrieve is available in the > > extent map, and is cached in em_cached for later retrieval. The extent > > map cached is also refcounted so it would not disappear. > > Limit the extent locking while reading the extnet maps only. The rest is > > just creating bio and fetching/decompressing the data according to the > > extent map provided. > > > > The only reason this was not working is because we would get EIO as > > the CRCs would not match (or were not committed to disk as yet) for > > folios that were written and released. In order to get over it, mark the > > extent as finished only after all folios are cleared of writeback. > > > > I have run the xfstests on it without any deadlocks or corruptions. > > However, a fresh outlook on what could go wrong with a limiting the > > scope of locks would be good. > > > > Goldwyn Rodrigues (2): > > btrfs: clear folio writeback after completing ordered range > > btrfs: take extent lock only while reading extent map > > The first patch seems relevant but it had been sent before Josef added > the read locking updates so I don't know if it still needs to be merged. > The second patch is covered by "btrfs: do not hold the extent lock for > entire read". The first patch is not relevant anymore because of the way extent locking is used for reads. ie, a read must make sure all ordered extents are submitted beore proceeding. This was not the case before and hence I used to receive CRC errors for reads. This was a small race, where a process would writeback and invalidate pages, and a fresh read is performed simultaneously. The CRC tree was not updated and reads would fail with EIO. The second patch is incorporated in Josef's v2 version of the patchset. In short, ignore this series.
From: Goldwyn Rodrigues <rgoldwyn@suse.com> Extent locking is not required for the entire read process. Once the extent map is read all information to retrieve is available in the extent map, and is cached in em_cached for later retrieval. The extent map cached is also refcounted so it would not disappear. Limit the extent locking while reading the extnet maps only. The rest is just creating bio and fetching/decompressing the data according to the extent map provided. The only reason this was not working is because we would get EIO as the CRCs would not match (or were not committed to disk as yet) for folios that were written and released. In order to get over it, mark the extent as finished only after all folios are cleared of writeback. I have run the xfstests on it without any deadlocks or corruptions. However, a fresh outlook on what could go wrong with a limiting the scope of locks would be good. Goldwyn Rodrigues (2): btrfs: clear folio writeback after completing ordered range btrfs: take extent lock only while reading extent map fs/btrfs/compression.c | 5 --- fs/btrfs/extent_io.c | 99 +++--------------------------------------- 2 files changed, 7 insertions(+), 97 deletions(-)