| Message ID | 20241018211541.42705-1-snitzer@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] nfs: avoid i_lock contention in nfs_clear_invalid_mapping | expand |
On Fri, 2024-10-18 at 17:15 -0400, Mike Snitzer wrote: > Multi-threaded buffered reads to the same file exposed significant > inode spinlock contention in nfs_clear_invalid_mapping(). > > Eliminate this spinlock contention by checking flags without locking, > instead using smp_rmb and smp_load_acquire accordingly, but then take > spinlock and double-check these inode flags. > > Also refactor nfs_set_cache_invalid() slightly to use > smp_store_release() to pair with nfs_clear_invalid_mapping()'s > smp_load_acquire(). > > While this fix is beneficial for all multi-threaded buffered reads > issued by an NFS client, this issue was identified in the context of > surprisingly low LOCALIO performance with 4K multi-threaded buffered > read IO. This fix dramatically speeds up LOCALIO performance: > > before: read: IOPS=1583k, BW=6182MiB/s (6482MB/s)(121GiB/20002msec) > after: read: IOPS=3046k, BW=11.6GiB/s (12.5GB/s)(232GiB/20001msec) > > Fixes: 17dfeb911339 ("NFS: Fix races in nfs_revalidate_mapping") > Signed-off-by: Mike Snitzer <snitzer@kernel.org> > --- > fs/nfs/inode.c | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > v2: fix issue Jeff Layton raised, need: flags |= nfsi->cache_validity; > > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c > index 542c7d97b235..cc7a32b32676 100644 > --- a/fs/nfs/inode.c > +++ b/fs/nfs/inode.c > @@ -205,12 +205,15 @@ void nfs_set_cache_invalid(struct inode *inode, unsigned long flags) > nfs_fscache_invalidate(inode, 0); > flags &= ~NFS_INO_REVAL_FORCED; > > - nfsi->cache_validity |= flags; > + flags |= nfsi->cache_validity; > + if (inode->i_mapping->nrpages == 0) > + flags &= ~NFS_INO_INVALID_DATA; > > - if (inode->i_mapping->nrpages == 0) { > - nfsi->cache_validity &= ~NFS_INO_INVALID_DATA; > - nfs_ooo_clear(nfsi); > - } else if (nfsi->cache_validity & NFS_INO_INVALID_DATA) { > + /* pairs with nfs_clear_invalid_mapping()'s smp_load_acquire() */ > + smp_store_release(&nfsi->cache_validity, flags); > + > + if (inode->i_mapping->nrpages == 0 || > + nfsi->cache_validity & NFS_INO_INVALID_DATA) { > nfs_ooo_clear(nfsi); > } > trace_nfs_set_cache_invalid(inode, 0); > @@ -1408,6 +1411,13 @@ int nfs_clear_invalid_mapping(struct address_space *mapping) > TASK_KILLABLE|TASK_FREEZABLE_UNSAFE); > if (ret) > goto out; > + smp_rmb(); /* pairs with smp_wmb() below */ > + if (test_bit(NFS_INO_INVALIDATING, bitlock)) > + continue; > + /* pairs with nfs_set_cache_invalid()'s smp_store_release() */ > + if (!(smp_load_acquire(&nfsi->cache_validity) & NFS_INO_INVALID_DATA)) > + goto out; > + /* Slow-path that double-checks with spinlock held */ > spin_lock(&inode->i_lock); > if (test_bit(NFS_INO_INVALIDATING, bitlock)) { > spin_unlock(&inode->i_lock); LGTM! Reviewed-by: Jeff Layton <jlayton@kernel.org>
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c index 542c7d97b235..cc7a32b32676 100644 --- a/fs/nfs/inode.c +++ b/fs/nfs/inode.c @@ -205,12 +205,15 @@ void nfs_set_cache_invalid(struct inode *inode, unsigned long flags) nfs_fscache_invalidate(inode, 0); flags &= ~NFS_INO_REVAL_FORCED; - nfsi->cache_validity |= flags; + flags |= nfsi->cache_validity; + if (inode->i_mapping->nrpages == 0) + flags &= ~NFS_INO_INVALID_DATA; - if (inode->i_mapping->nrpages == 0) { - nfsi->cache_validity &= ~NFS_INO_INVALID_DATA; - nfs_ooo_clear(nfsi); - } else if (nfsi->cache_validity & NFS_INO_INVALID_DATA) { + /* pairs with nfs_clear_invalid_mapping()'s smp_load_acquire() */ + smp_store_release(&nfsi->cache_validity, flags); + + if (inode->i_mapping->nrpages == 0 || + nfsi->cache_validity & NFS_INO_INVALID_DATA) { nfs_ooo_clear(nfsi); } trace_nfs_set_cache_invalid(inode, 0); @@ -1408,6 +1411,13 @@ int nfs_clear_invalid_mapping(struct address_space *mapping) TASK_KILLABLE|TASK_FREEZABLE_UNSAFE); if (ret) goto out; + smp_rmb(); /* pairs with smp_wmb() below */ + if (test_bit(NFS_INO_INVALIDATING, bitlock)) + continue; + /* pairs with nfs_set_cache_invalid()'s smp_store_release() */ + if (!(smp_load_acquire(&nfsi->cache_validity) & NFS_INO_INVALID_DATA)) + goto out; + /* Slow-path that double-checks with spinlock held */ spin_lock(&inode->i_lock); if (test_bit(NFS_INO_INVALIDATING, bitlock)) { spin_unlock(&inode->i_lock);
Multi-threaded buffered reads to the same file exposed significant inode spinlock contention in nfs_clear_invalid_mapping(). Eliminate this spinlock contention by checking flags without locking, instead using smp_rmb and smp_load_acquire accordingly, but then take spinlock and double-check these inode flags. Also refactor nfs_set_cache_invalid() slightly to use smp_store_release() to pair with nfs_clear_invalid_mapping()'s smp_load_acquire(). While this fix is beneficial for all multi-threaded buffered reads issued by an NFS client, this issue was identified in the context of surprisingly low LOCALIO performance with 4K multi-threaded buffered read IO. This fix dramatically speeds up LOCALIO performance: before: read: IOPS=1583k, BW=6182MiB/s (6482MB/s)(121GiB/20002msec) after: read: IOPS=3046k, BW=11.6GiB/s (12.5GB/s)(232GiB/20001msec) Fixes: 17dfeb911339 ("NFS: Fix races in nfs_revalidate_mapping") Signed-off-by: Mike Snitzer <snitzer@kernel.org> --- fs/nfs/inode.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) v2: fix issue Jeff Layton raised, need: flags |= nfsi->cache_validity;