On Tue, Feb 3, 2015 at 5:47 PM, Kornievskaia, Olga
<Olga.Kornievskaia@xxxxxxxxxx> wrote:
>
>> On Feb 3, 2015, at 4:59 PM, Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx> wrote:
>>
>> If we already hold a delegation, there should be no reason for the
>> server to issue it to us again. Unfortunately, there appear to be
>> servers out there that engage in this practice. While it is often
>> harmless to do so, there is one case where this creates a problem
>> and that is when the client is in the process of returning that
>> delegation.
>> This patch uses the NFSv4.1 NFS4_SHARE_WANT_NO_DELEG flag to inform
>> the server not to return a delegation in these cases.
>>
>> Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
>> ---
>> fs/nfs/nfs4proc.c | 25 ++++++++++++++++++++++---
>> 1 file changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index cd4295d84d54..fb41624bafc9 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -942,8 +942,10 @@ static bool nfs4_clear_cap_atomic_open_v1(struct nfs_server *server,
>>
>> static u32
>> nfs4_map_atomic_open_share(struct nfs_server *server,
>> + struct inode *inode,
>> fmode_t fmode, int openflags)
>> {
>> + struct nfs_delegation *delegation;
>> u32 res = 0;
>>
>> switch (fmode & (FMODE_READ | FMODE_WRITE)) {
>> @@ -959,8 +961,25 @@ nfs4_map_atomic_open_share(struct nfs_server *server,
>> if (!(server->caps & NFS_CAP_ATOMIC_OPEN_V1))
>> goto out;
>> /* Want no delegation if we're using O_DIRECT */
>> - if (openflags & O_DIRECT)
>> + if (openflags & O_DIRECT) {
>> res |= NFS4_SHARE_WANT_NO_DELEG;
>> + goto out;
>> + }
>> + if (inode == NULL)
>> + goto out;
>> + rcu_read_lock();
>> + delegation = rcu_dereference(NFS_I(inode)->delegation);
>> + /*
>> + * If we have a delegation, either ask for an upgrade or ask for
>> + * no delegation
>> + */
>> + if (delegation) {
>> + if ((fmode & FMODE_WRITE) && !(delegation->type & FMODE_WRITE))
>> + res |= NFS4_SHARE_WANT_WRITE_DELEG;
>> + else
>> + res |= NFS4_SHARE_WANT_NO_DELEG;
>> + }
>> + rcu_read_unlock();
>> out:
>> return res;
>> }
>> @@ -1028,7 +1047,7 @@ static struct nfs4_opendata *nfs4_opendata_alloc(struct dentry *dentry,
>> p->o_arg.open_flags = flags;
>> p->o_arg.fmode = fmode & (FMODE_READ|FMODE_WRITE);
>> p->o_arg.share_access = nfs4_map_atomic_open_share(server,
>> - fmode, flags);
>> + dentry->d_inode, fmode, flags);
>> /* don't put an ACCESS op in OPEN compound if O_EXCL, because ACCESS
>> * will return permission denied for all bits until close */
>> if (!(flags & O_EXCL)) {
>> @@ -2724,7 +2743,7 @@ static void nfs4_close_prepare(struct rpc_task *task, void *data)
>> }
>> calldata->arg.share_access =
>> nfs4_map_atomic_open_share(NFS_SERVER(inode),
>> - calldata->arg.fmode, 0);
>> + NULL, calldata->arg.fmode, 0);
>>
>> nfs_fattr_init(calldata->res.fattr);
>> calldata->timestamp = jiffies;
>> --
>> 2.1.0
>>
>
>
> Hi Trond,
>
> Do you see this of helping with the race? We won’t find a delegation on the racing open as it has been detached from the inode. For the other patch, aren’t we already checking if we can do a local open by checking for the delegation (can_open_delegated())?
>
Argh. You're 100% right in that it won't completely close the race.
However it narrows the race to the delegreturn itself, whereas
currently we also have a race while in the process of reclaiming opens
+ locks.
As for patch 1/2, I believe that O_DIRECT opens are still a valid case.
--
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@xxxxxxxxxxxxxxx
--
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
