| 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).
Hi Mike, On 10/18/24 5:37 PM, Mike Snitzer wrote: > 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'm having some issues with non-localio NFS after applying this patch: - cthon basic tests fail with NFS v3 - cthon general tests fail with NFS v4.1 and v4.2 - xfstests generic/080, generic/472, generic/615, and generic/633 fail with NFS v4.1 and v4.2 - xfstests generic/683, and generic/684 fail with NFS v4.2 I think the problem is the call to smp_store_release(). It's overwriting nfsi->cache_validity with the value of 'flags', losing anything set there but not in 'flags'. Could we instead do something like: smp_store_release(&nfsi->cache_validity, nfsi->cache_validity | flags) ? Anna >>> 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).
On Wed, Oct 30, 2024 at 03:51:44PM -0400, Anna Schumaker wrote: > Hi Mike, > > On 10/18/24 5:37 PM, Mike Snitzer wrote: > > 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'm having some issues with non-localio NFS after applying this patch: > > - cthon basic tests fail with NFS v3 > - cthon general tests fail with NFS v4.1 and v4.2 > - xfstests generic/080, generic/472, generic/615, and generic/633 fail with NFS v4.1 and v4.2 > - xfstests generic/683, and generic/684 fail with NFS v4.2 > > I think the problem is the call to smp_store_release(). It's overwriting nfsi->cache_validity > with the value of 'flags', losing anything set there but not in 'flags'. Could we instead do > something like: > smp_store_release(&nfsi->cache_validity, nfsi->cache_validity | flags) > ? > > Anna Hi, v2 addressed this issue like Jeff suggested with: > >>> > >>> flags |= nfsi->cache_validity; > >>> smp_store_release(&nfsi->cache_validity, flags); > >> > >> Ah good catch, sorry about that, will fix. I think you must not be using v2? https://lore.kernel.org/all/20241018211541.42705-1-snitzer@kernel.org/ Jeff also provided his Reviewed-by for v2. If you are using v2 that'll be weird (because I'm not seeing any issues with xfstests, etc). Thanks, Mike
On 10/30/24 4:03 PM, Mike Snitzer wrote: > On Wed, Oct 30, 2024 at 03:51:44PM -0400, Anna Schumaker wrote: >> Hi Mike, >> >> On 10/18/24 5:37 PM, Mike Snitzer wrote: >>> 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'm having some issues with non-localio NFS after applying this patch: >> >> - cthon basic tests fail with NFS v3 >> - cthon general tests fail with NFS v4.1 and v4.2 >> - xfstests generic/080, generic/472, generic/615, and generic/633 fail with NFS v4.1 and v4.2 >> - xfstests generic/683, and generic/684 fail with NFS v4.2 >> >> I think the problem is the call to smp_store_release(). It's overwriting nfsi->cache_validity >> with the value of 'flags', losing anything set there but not in 'flags'. Could we instead do >> something like: >> smp_store_release(&nfsi->cache_validity, nfsi->cache_validity | flags) >> ? >> >> Anna > > Hi, > > v2 addressed this issue like Jeff suggested with: > >>>>> >>>>> flags |= nfsi->cache_validity; >>>>> smp_store_release(&nfsi->cache_validity, flags); >>>> >>>> Ah good catch, sorry about that, will fix. > > I think you must not be using v2? Oops! It looks like `b4` grabbed v1 of the patch from the mailing list instead of v2. > > https://lore.kernel.org/all/20241018211541.42705-1-snitzer@kernel.org/ > > Jeff also provided his Reviewed-by for v2. > > If you are using v2 that'll be weird (because I'm not seeing any > issues with xfstests, etc). v2 does fix the issues I was seeing. Sorry about the noise! Anna > > Thanks, > Mike
On 10/30/24 4:03 PM, Mike Snitzer wrote: > On Wed, Oct 30, 2024 at 03:51:44PM -0400, Anna Schumaker wrote: >> Hi Mike, >> >> On 10/18/24 5:37 PM, Mike Snitzer wrote: >>> 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'm having some issues with non-localio NFS after applying this patch: >> >> - cthon basic tests fail with NFS v3 >> - cthon general tests fail with NFS v4.1 and v4.2 >> - xfstests generic/080, generic/472, generic/615, and generic/633 fail with NFS v4.1 and v4.2 >> - xfstests generic/683, and generic/684 fail with NFS v4.2 >> >> I think the problem is the call to smp_store_release(). It's overwriting nfsi->cache_validity >> with the value of 'flags', losing anything set there but not in 'flags'. Could we instead do >> something like: >> smp_store_release(&nfsi->cache_validity, nfsi->cache_validity | flags) >> ? >> >> Anna > > Hi, > > v2 addressed this issue like Jeff suggested with: > >>>>> >>>>> flags |= nfsi->cache_validity; >>>>> smp_store_release(&nfsi->cache_validity, flags); >>>> >>>> Ah good catch, sorry about that, will fix. > > I think you must not be using v2? Oops! It looks like `b4` grabbed v1 of the patch from the mailing list instead of v2. > > https://lore.kernel.org/all/20241018211541.42705-1-snitzer@kernel.org/ > > Jeff also provided his Reviewed-by for v2. > > If you are using v2 that'll be weird (because I'm not seeing any > issues with xfstests, etc). v2 does fix the issues I was seeing. Sorry about the noise! Anna > > Thanks, > Mike
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(-)