On Dec. 16, 2009, 23:18 +0200, " J. Bruce Fields" <bfields@xxxxxxxxxxxxxx> wrote:
> On Wed, Dec 16, 2009 at 07:42:29PM +0200, Benny Halevy wrote:
>> as dl_vfs_file may be used while the delegation structure is
>> being referenced.
>
> A callback to the client (which can last a while if the client is
> unresponsive) can also hold a reference to the delegation. It doesn't
> need a reference on the file.
>
> I guess I can't see any bug in holding a reference on the file in that
> case, but it seems better not to (if only to allow freeing it earlier).
>
> Maybe the limited amount of information used by the callback should be
> reference-counted separately. I'm not sure.
>
If we lock the delegation, say by grabbing the global deleg_lock
(or a fine-grained per-deleg lock) we can make sure dl_vfs_file
is accessed atomically in nfs4_new_open and nfs4_preprocess_stateid_op
vs. being freed in close_delegation.
That said, it looks like get_file on the *filpp nfs4_preprocess_stateid_op
is setting (on both legs of the is_delegation_stateid() condition)
should happen inside nfs4_preprocess_stateid_op rather while it holds
a ref count on the delegation (and under the lock proposed above)
rather than by its callers, nfsd4_read and nfsd4_write.
Benny
> --b.
>
>> Signed-off-by: Benny Halevy <bhalevy@xxxxxxxxxxx>
>> ---
>> fs/nfsd/nfs4state.c | 27 +++++++++++++--------------
>> 1 files changed, 13 insertions(+), 14 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 411226a..053fd2b 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -203,29 +203,17 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_stateid *stp, struct svc_f
>> return dp;
>> }
>>
>> -void
>> -nfs4_put_delegation(struct nfs4_delegation *dp)
>> -{
>> - if (atomic_dec_and_test(&dp->dl_count)) {
>> - dprintk("NFSD: freeing dp %p\n",dp);
>> - put_nfs4_file(dp->dl_file);
>> - kmem_cache_free(deleg_slab, dp);
>> - num_delegations--;
>> - }
>> -}
>> -
>> /* Remove the associated file_lock first, then remove the delegation.
>> * lease_modify() is called to remove the FS_LEASE file_lock from
>> * the i_flock list, eventually calling nfsd's lock_manager
>> * fl_release_callback.
>> */
>> static void
>> -nfs4_close_delegation(struct nfs4_delegation *dp)
>> +__close_delegation(struct nfs4_delegation *dp)
>> {
>> struct file *filp = dp->dl_vfs_file;
>>
>> dprintk("NFSD: close_delegation dp %p\n",dp);
>> - dp->dl_vfs_file = NULL;
>> /* The following nfsd_close may not actually close the file,
>> * but we want to remove the lease in any case. */
>> if (dp->dl_flock)
>> @@ -233,6 +221,18 @@ nfs4_close_delegation(struct nfs4_delegation *dp)
>> nfsd_close(filp);
>> }
>>
>> +void
>> +nfs4_put_delegation(struct nfs4_delegation *dp)
>> +{
>> + if (atomic_dec_and_test(&dp->dl_count)) {
>> + dprintk("NFSD: freeing dp %p\n",dp);
>> + __close_delegation(dp);
>> + put_nfs4_file(dp->dl_file);
>> + kmem_cache_free(deleg_slab, dp);
>> + num_delegations--;
>> + }
>> +}
>> +
>> /* Called under the state lock. */
>> static void
>> unhash_delegation(struct nfs4_delegation *dp)
>> @@ -242,7 +242,6 @@ unhash_delegation(struct nfs4_delegation *dp)
>> list_del_init(&dp->dl_perclnt);
>> list_del_init(&dp->dl_recall_lru);
>> spin_unlock(&deleg_lock);
>> - nfs4_close_delegation(dp);
>> nfs4_put_delegation(dp);
>> }
>>
>> --
>> 1.6.5.1
>>
--
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
