| Message ID | 20241018170335.41427-1-snitzer@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nfs: avoid i_lock contention in nfs_clear_invalid_mapping | expand |
On Fri, 2024-10-18 at 13:03 -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 | 19 ++++++++++++++----- > 1 file changed, 14 insertions(+), 5 deletions(-) > > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c > index 542c7d97b235..130d7226b12a 100644 > --- a/fs/nfs/inode.c > +++ b/fs/nfs/inode.c > @@ -205,12 +205,14 @@ 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; > + 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); > + I don't know this code that well, but it used to do an |= of flags into cache_validity. Now you're replacing cache_validity wholesale with flags. Maybe that should do something like this? flags |= nfsi->cache_validity; 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 +1410,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);
On Fri, Oct 18, 2024 at 03:39:13PM -0400, Jeff Layton wrote: > On Fri, 2024-10-18 at 13:03 -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 | 19 ++++++++++++++----- > > 1 file changed, 14 insertions(+), 5 deletions(-) > > > > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c > > index 542c7d97b235..130d7226b12a 100644 > > --- a/fs/nfs/inode.c > > +++ b/fs/nfs/inode.c > > @@ -205,12 +205,14 @@ 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; > > + 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); > > + > > I don't know this code that well, but it used to do an |= of flags into > cache_validity. Now you're replacing cache_validity wholesale with > flags. Maybe that should do something like this? > > flags |= nfsi->cache_validity; > smp_store_release(&nfsi->cache_validity, flags); Ah good catch, sorry about that, will fix. This will allow further cleanup too, will let v2 speak to that. Thanks! Mike
On Fri, Oct 18, 2024 at 03:49:50PM -0400, Mike Snitzer wrote: > On Fri, Oct 18, 2024 at 03:39:13PM -0400, Jeff Layton wrote: > > On Fri, 2024-10-18 at 13:03 -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 | 19 ++++++++++++++----- > > > 1 file changed, 14 insertions(+), 5 deletions(-) > > > > > > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c > > > index 542c7d97b235..130d7226b12a 100644 > > > --- a/fs/nfs/inode.c > > > +++ b/fs/nfs/inode.c > > > @@ -205,12 +205,14 @@ 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; > > > + 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); > > > + > > > > I don't know this code that well, but it used to do an |= of flags into > > cache_validity. Now you're replacing cache_validity wholesale with > > flags. Maybe that should do something like this? > > > > flags |= nfsi->cache_validity; > > smp_store_release(&nfsi->cache_validity, flags); > > Ah good catch, sorry about that, will fix. > > This will allow further cleanup too, will let v2 speak to that. I just posted v2, but for completeness: I decided to leave well enough alone and not do any further cleanup. (my thinking was relative to nfs_ooo_clear, and possibly moving it before the smp_store_release, so that nfsi->cache_validity only changed once using smp_store_release. The interlock between nfs_set_cache_invalid and nfs_clear_invalid_mapping is focused on NFS_INO_INVALID_DATA, which nfs_ooo_clear doesn't touch).
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c index 542c7d97b235..130d7226b12a 100644 --- a/fs/nfs/inode.c +++ b/fs/nfs/inode.c @@ -205,12 +205,14 @@ 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; + 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 +1410,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 | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)