| Message ID | 20210430050900.106851-1-dai.ngo@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] NFSv4: can_open_cached needs to be called with so_lock | expand |
On Fri, 2021-04-30 at 01:09 -0400, Dai Ngo wrote: > Currently can_open_cached accesses the openstate's flags without the > so_lock and also does not update the flags of the cached state. This > results in the openstate's flags be out of sync which can cause the > file to be closed prematurely. > > This patch adds the missing so_lock around the call to > can_open_cached > and also updates the openstate's flags if the cached openstate is > used. > > Signed-off-by: Dai Ngo <dai.ngo@oracle.com> > --- > fs/nfs/nfs4proc.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index c65c4b41e2c1..2464e77c51f9 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -2410,9 +2410,15 @@ static void nfs4_open_prepare(struct rpc_task > *task, void *calldata) > if (data->state != NULL) { > struct nfs_delegation *delegation; > > + spin_lock(&data->state->owner->so_lock); > if (can_open_cached(data->state, data->o_arg.fmode, > - data->o_arg.open_flags, > claim)) > + data->o_arg.open_flags, claim)) { > + update_open_stateflags(data->state, data- > >o_arg.fmode); > + spin_unlock(&data->state->owner->so_lock); > goto out_no_action; > + } > + spin_unlock(&data->state->owner->so_lock); > + > rcu_read_lock(); > delegation = nfs4_get_valid_delegation(data->state- > >inode); > if (can_open_delegated(delegation, data->o_arg.fmode, > claim)) This is going to introduce stateid leaks. The actual update of the open state flags happens in nfs4_try_open_cached(), which is called from nfs4_opendata_to_nfs4_state(). While we could put spinlocks around the call to can_open_cached() here, there is little point in doing so, since this is just a read-only advisory check. The real check is performed, as I said, in nfs4_try_open_cached().
On 4/30/21 5:42 AM, Trond Myklebust wrote: > On Fri, 2021-04-30 at 01:09 -0400, Dai Ngo wrote: >> Currently can_open_cached accesses the openstate's flags without the >> so_lock and also does not update the flags of the cached state. This >> results in the openstate's flags be out of sync which can cause the >> file to be closed prematurely. >> >> This patch adds the missing so_lock around the call to >> can_open_cached >> and also updates the openstate's flags if the cached openstate is >> used. >> >> Signed-off-by: Dai Ngo <dai.ngo@oracle.com> >> --- >> fs/nfs/nfs4proc.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >> index c65c4b41e2c1..2464e77c51f9 100644 >> --- a/fs/nfs/nfs4proc.c >> +++ b/fs/nfs/nfs4proc.c >> @@ -2410,9 +2410,15 @@ static void nfs4_open_prepare(struct rpc_task >> *task, void *calldata) >> if (data->state != NULL) { >> struct nfs_delegation *delegation; >> >> + spin_lock(&data->state->owner->so_lock); >> if (can_open_cached(data->state, data->o_arg.fmode, >> - data->o_arg.open_flags, >> claim)) >> + data->o_arg.open_flags, claim)) { >> + update_open_stateflags(data->state, data- >>> o_arg.fmode); >> + spin_unlock(&data->state->owner->so_lock); >> goto out_no_action; >> + } >> + spin_unlock(&data->state->owner->so_lock); >> + >> rcu_read_lock(); >> delegation = nfs4_get_valid_delegation(data->state- >>> inode); >> if (can_open_delegated(delegation, data->o_arg.fmode, >> claim)) > This is going to introduce stateid leaks. The actual update of the open > state flags happens in nfs4_try_open_cached(), which is called from > nfs4_opendata_to_nfs4_state(). Right, the actual update is done by _nfs4_opendata_to_nfs4_state called from _nfs4_do_open/_nfs4_open_and_get_state. I missed the check of data->cancelled in nfs4_open_release and just keying in on rpc_done not set path which skips the call to nfs4_opendata_to_nfs4_state. Thanks Trond! -Dai > > While we could put spinlocks around the call to can_open_cached() here, > there is little point in doing so, since this is just a read-only > advisory check. The real check is performed, as I said, in > nfs4_try_open_cached(). >
Hi Trond, I have a question below: On 4/30/21 5:42 AM, Trond Myklebust wrote: > On Fri, 2021-04-30 at 01:09 -0400, Dai Ngo wrote: >> Currently can_open_cached accesses the openstate's flags without the >> so_lock and also does not update the flags of the cached state. This >> results in the openstate's flags be out of sync which can cause the >> file to be closed prematurely. >> >> This patch adds the missing so_lock around the call to >> can_open_cached >> and also updates the openstate's flags if the cached openstate is >> used. >> >> Signed-off-by: Dai Ngo <dai.ngo@oracle.com> >> --- >> fs/nfs/nfs4proc.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >> index c65c4b41e2c1..2464e77c51f9 100644 >> --- a/fs/nfs/nfs4proc.c >> +++ b/fs/nfs/nfs4proc.c >> @@ -2410,9 +2410,15 @@ static void nfs4_open_prepare(struct rpc_task >> *task, void *calldata) >> if (data->state != NULL) { >> struct nfs_delegation *delegation; >> >> + spin_lock(&data->state->owner->so_lock); >> if (can_open_cached(data->state, data->o_arg.fmode, >> - data->o_arg.open_flags, >> claim)) >> + data->o_arg.open_flags, claim)) { >> + update_open_stateflags(data->state, data- >>> o_arg.fmode); >> + spin_unlock(&data->state->owner->so_lock); >> goto out_no_action; >> + } >> + spin_unlock(&data->state->owner->so_lock); >> + >> rcu_read_lock(); >> delegation = nfs4_get_valid_delegation(data->state- >>> inode); >> if (can_open_delegated(delegation, data->o_arg.fmode, >> claim)) > This is going to introduce stateid leaks. The actual update of the open > state flags happens in nfs4_try_open_cached(), which is called from > nfs4_opendata_to_nfs4_state(). > > While we could put spinlocks around the call to can_open_cached() here, > there is little point in doing so, since this is just a read-only > advisory check. The real check is performed, as I said, in > nfs4_try_open_cached(). If we wait to update the flags in _nfs4_opendata_to_nfs4_state after the RPC thread decides to use the cached state, the file could be closed by another thread before _nfs4_opendata_to_nfs4_state is called by another thread. The client in this case will retry the open from nfs4_do_open and everything is ok. However, if we update the flags nfs4_open_prepare then it will prevent the file from being closed and this saves one CLOSE and one OPEN rpc request to the server. Is this correct and is it worth it to consider doing anything since this is a rare scenario? Thanks, -Dai >
On Fri, 2021-04-30 at 10:24 -0700, dai.ngo@oracle.com wrote: > Hi Trond, > > I have a question below: > > On 4/30/21 5:42 AM, Trond Myklebust wrote: > > On Fri, 2021-04-30 at 01:09 -0400, Dai Ngo wrote: > > > Currently can_open_cached accesses the openstate's flags without > > > the > > > so_lock and also does not update the flags of the cached state. > > > This > > > results in the openstate's flags be out of sync which can cause > > > the > > > file to be closed prematurely. > > > > > > This patch adds the missing so_lock around the call to > > > can_open_cached > > > and also updates the openstate's flags if the cached openstate is > > > used. > > > > > > Signed-off-by: Dai Ngo <dai.ngo@oracle.com> > > > --- > > > fs/nfs/nfs4proc.c | 8 +++++++- > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > > > index c65c4b41e2c1..2464e77c51f9 100644 > > > --- a/fs/nfs/nfs4proc.c > > > +++ b/fs/nfs/nfs4proc.c > > > @@ -2410,9 +2410,15 @@ static void nfs4_open_prepare(struct > > > rpc_task > > > *task, void *calldata) > > > if (data->state != NULL) { > > > struct nfs_delegation *delegation; > > > > > > + spin_lock(&data->state->owner->so_lock); > > > if (can_open_cached(data->state, data- > > > >o_arg.fmode, > > > - data->o_arg.open_flags, > > > claim)) > > > + data->o_arg.open_flags, claim)) { > > > + update_open_stateflags(data->state, data- > > > > o_arg.fmode); > > > + spin_unlock(&data->state->owner- > > > >so_lock); > > > goto out_no_action; > > > + } > > > + spin_unlock(&data->state->owner->so_lock); > > > + > > > rcu_read_lock(); > > > delegation = nfs4_get_valid_delegation(data- > > > >state- > > > > inode); > > > if (can_open_delegated(delegation, data- > > > >o_arg.fmode, > > > claim)) > > This is going to introduce stateid leaks. The actual update of the > > open > > state flags happens in nfs4_try_open_cached(), which is called from > > nfs4_opendata_to_nfs4_state(). > > > > While we could put spinlocks around the call to can_open_cached() > > here, > > there is little point in doing so, since this is just a read-only > > advisory check. The real check is performed, as I said, in > > nfs4_try_open_cached(). > > If we wait to update the flags in _nfs4_opendata_to_nfs4_state after > the > RPC thread decides to use the cached state, the file could be closed > by > another thread before _nfs4_opendata_to_nfs4_state is called by > another > thread. The client in this case will retry the open from nfs4_do_open > and > everything is ok. > > However, if we update the flags nfs4_open_prepare then it will > prevent > the file from being closed and this saves one CLOSE and one OPEN rpc > request to the server. Is this correct and is it worth it to > consider > doing anything since this is a rare scenario? > > If you're in a scenario where several processes are accessing the same file on the same NFS client, you probably want to see the server hand out a delegation for that file rather than keep relying on OPEN/CLOSE. That's actually why we started using nfs4_try_open_cached(). The intention was that it mainly manages the delegated open case. We then added support for the non-delegated case mainly because the Linux server doesn't support write delegations and because there were corner cases where files were being opened/closed by multiple processes without a delegation. So what I'm saying is that ideally we really want to concentrate on fixing the Linux server to support write delegations so that we can relegate most of this code to handling corner cases. Make sense?
On 4/30/21 10:56 AM, Trond Myklebust wrote: > On Fri, 2021-04-30 at 10:24 -0700, dai.ngo@oracle.com wrote: >> Hi Trond, >> >> I have a question below: >> >> On 4/30/21 5:42 AM, Trond Myklebust wrote: >>> On Fri, 2021-04-30 at 01:09 -0400, Dai Ngo wrote: >>>> Currently can_open_cached accesses the openstate's flags without >>>> the >>>> so_lock and also does not update the flags of the cached state. >>>> This >>>> results in the openstate's flags be out of sync which can cause >>>> the >>>> file to be closed prematurely. >>>> >>>> This patch adds the missing so_lock around the call to >>>> can_open_cached >>>> and also updates the openstate's flags if the cached openstate is >>>> used. >>>> >>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com> >>>> --- >>>> fs/nfs/nfs4proc.c | 8 +++++++- >>>> 1 file changed, 7 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >>>> index c65c4b41e2c1..2464e77c51f9 100644 >>>> --- a/fs/nfs/nfs4proc.c >>>> +++ b/fs/nfs/nfs4proc.c >>>> @@ -2410,9 +2410,15 @@ static void nfs4_open_prepare(struct >>>> rpc_task >>>> *task, void *calldata) >>>> if (data->state != NULL) { >>>> struct nfs_delegation *delegation; >>>> >>>> + spin_lock(&data->state->owner->so_lock); >>>> if (can_open_cached(data->state, data- >>>>> o_arg.fmode, >>>> - data->o_arg.open_flags, >>>> claim)) >>>> + data->o_arg.open_flags, claim)) { >>>> + update_open_stateflags(data->state, data- >>>>> o_arg.fmode); >>>> + spin_unlock(&data->state->owner- >>>>> so_lock); >>>> goto out_no_action; >>>> + } >>>> + spin_unlock(&data->state->owner->so_lock); >>>> + >>>> rcu_read_lock(); >>>> delegation = nfs4_get_valid_delegation(data- >>>>> state- >>>>> inode); >>>> if (can_open_delegated(delegation, data- >>>>> o_arg.fmode, >>>> claim)) >>> This is going to introduce stateid leaks. The actual update of the >>> open >>> state flags happens in nfs4_try_open_cached(), which is called from >>> nfs4_opendata_to_nfs4_state(). >>> >>> While we could put spinlocks around the call to can_open_cached() >>> here, >>> there is little point in doing so, since this is just a read-only >>> advisory check. The real check is performed, as I said, in >>> nfs4_try_open_cached(). >> If we wait to update the flags in _nfs4_opendata_to_nfs4_state after >> the >> RPC thread decides to use the cached state, the file could be closed >> by >> another thread before _nfs4_opendata_to_nfs4_state is called by >> another >> thread. The client in this case will retry the open from nfs4_do_open >> and >> everything is ok. >> >> However, if we update the flags nfs4_open_prepare then it will >> prevent >> the file from being closed and this saves one CLOSE and one OPEN rpc >> request to the server. Is this correct and is it worth it to >> consider >> doing anything since this is a rare scenario? > If you're in a scenario where several processes are accessing the same > file on the same NFS client, you probably want to see the server hand > out a delegation for that file rather than keep relying on OPEN/CLOSE. > That's actually why we started using nfs4_try_open_cached(). The > intention was that it mainly manages the delegated open case. We then > added support for the non-delegated case mainly because the Linux > server doesn't support write delegations and because there were corner > cases where files were being opened/closed by multiple processes > without a delegation. > > So what I'm saying is that ideally we really want to concentrate on > fixing the Linux server to support write delegations so that we can > relegate most of this code to handling corner cases. > > Make sense? Yes, I agreed. It's not worth the effort to look and fix corner cases on the client side that rarely happen. Support for write delegations provides much more benefits and it's worth to spend the effort on. Thanks, -Dai
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index c65c4b41e2c1..2464e77c51f9 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -2410,9 +2410,15 @@ static void nfs4_open_prepare(struct rpc_task *task, void *calldata) if (data->state != NULL) { struct nfs_delegation *delegation; + spin_lock(&data->state->owner->so_lock); if (can_open_cached(data->state, data->o_arg.fmode, - data->o_arg.open_flags, claim)) + data->o_arg.open_flags, claim)) { + update_open_stateflags(data->state, data->o_arg.fmode); + spin_unlock(&data->state->owner->so_lock); goto out_no_action; + } + spin_unlock(&data->state->owner->so_lock); + rcu_read_lock(); delegation = nfs4_get_valid_delegation(data->state->inode); if (can_open_delegated(delegation, data->o_arg.fmode, claim))
Currently can_open_cached accesses the openstate's flags without the so_lock and also does not update the flags of the cached state. This results in the openstate's flags be out of sync which can cause the file to be closed prematurely. This patch adds the missing so_lock around the call to can_open_cached and also updates the openstate's flags if the cached openstate is used. Signed-off-by: Dai Ngo <dai.ngo@oracle.com> --- fs/nfs/nfs4proc.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)