On Tue, 28 Jan 2014 09:49:10 -0500
Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx> wrote:
> Commit d529ef83c355f97027ff85298a9709fe06216a66 (NFS: fix the handling
> of NFS_INO_INVALID_DATA flag in nfs_revalidate_mapping) introduces
> a potential race, since it doesn't test the value of nfsi->cache_validity
> and set the bitlock in nfsi->flags atomically.
>
> Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> Cc: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
> fs/nfs/inode.c | 22 ++++++++++------------
> 1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index 0a972ee9ccc1..8a5bcb6040ac 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -1038,24 +1038,22 @@ int nfs_revalidate_mapping(struct inode *inode, struct address_space *mapping)
> nfs_wait_bit_killable, TASK_KILLABLE);
> if (ret)
> goto out;
> - if (!(nfsi->cache_validity & NFS_INO_INVALID_DATA))
> + spin_lock(&inode->i_lock);
> + if (!(nfsi->cache_validity & NFS_INO_INVALID_DATA)) {
> + spin_unlock(&inode->i_lock);
> goto out;
> + }
> if (!test_and_set_bit_lock(NFS_INO_INVALIDATING, bitlock))
> break;
> - }
> -
I don't think that patch will help.
Consider the case where we have two tasks that enter
nfs_revalidate_mapping simultaneously. NFS_INO_INVALIDATING is
initially clear and NFS_INO_INVALID_DATA is set:
Task 1 Task2
---------------------------------------------------------------------
wait_on_bit(NFS_INO_INVALIDATING) wait_on_bit(NFS_INO_INVALIDATING)
gets i_lock spins on i_lock
checks NFS_INO_INVALID_DATA
sets NFS_INO_INVALIDATING
clears NFS_INO_INVALID_DATA
drops i_lock gets i_lock, finds that NFS_INO_INVALID_DATA
is clear, and exits the function
calls nfs_invalidate_mapping
...so task2 just returned from nfs_revalidate_mapping before the pages
got invalidated.
> - spin_lock(&inode->i_lock);
> - if (nfsi->cache_validity & NFS_INO_INVALID_DATA) {
> - nfsi->cache_validity &= ~NFS_INO_INVALID_DATA;
> - spin_unlock(&inode->i_lock);
> - trace_nfs_invalidate_mapping_enter(inode);
> - ret = nfs_invalidate_mapping(inode, mapping);
> - trace_nfs_invalidate_mapping_exit(inode, ret);
> - } else {
> - /* something raced in and cleared the flag */
> spin_unlock(&inode->i_lock);
> }
>
> + nfsi->cache_validity &= ~NFS_INO_INVALID_DATA;
> + spin_unlock(&inode->i_lock);
> + trace_nfs_invalidate_mapping_enter(inode);
> + ret = nfs_invalidate_mapping(inode, mapping);
> + trace_nfs_invalidate_mapping_exit(inode, ret);
> +
> clear_bit_unlock(NFS_INO_INVALIDATING, bitlock);
> smp_mb__after_clear_bit();
> wake_up_bit(bitlock, NFS_INO_INVALIDATING);
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
