| Message ID | 20121116165845.GA32183@fieldses.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
16.11.2012 20:58, bfields@fieldses.org ?????: > > A patch follows: note it's a two-line patch, with 20 lines of changelog > showing that I looked at what state might be shared by other threads and > explaining why I think this is safe. > Acked-by: Stanislav Kinsburskiy <skinsbursky@parallels.com> > I think that's what we need to do: little patches that remove it from > one or another part of the code with careful explanation of why it > works. > Yes, thanks. I'll also try to simplify nfsd_open() by little patches. In general it looks like client_mutex protect two different things: open owner state (which is containerised already, actually) and files access. So, probably, this client mutex have to be converted into two: per-net one, which protects open owner state, and static one, which protects files operations. What do you think about it?
On Mon, Nov 19, 2012 at 12:39:23PM +0400, Stanislav Kinsbursky wrote: > 16.11.2012 20:58, bfields@fieldses.org ?????: > > > >A patch follows: note it's a two-line patch, with 20 lines of changelog > >showing that I looked at what state might be shared by other threads and > >explaining why I think this is safe. > > > > Acked-by: Stanislav Kinsburskiy <skinsbursky@parallels.com> > > >I think that's what we need to do: little patches that remove it from > >one or another part of the code with careful explanation of why it > >works. > > > > Yes, thanks. I'll also try to simplify nfsd_open() by little patches. > In general it looks like client_mutex protect two different things: > open owner state (which is containerised already, actually) and > files access. > So, probably, this client mutex have to be converted into two: > per-net one, which protects open owner state, and static one, which > protects files operations. > What do you think about it? That sounds right. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
19.11.2012 16:46, bfields@fieldses.org ?????: > On Mon, Nov 19, 2012 at 12:39:23PM +0400, Stanislav Kinsbursky wrote: >> 16.11.2012 20:58, bfields@fieldses.org ?????: >>> >>> A patch follows: note it's a two-line patch, with 20 lines of changelog >>> showing that I looked at what state might be shared by other threads and >>> explaining why I think this is safe. >>> >> >> Acked-by: Stanislav Kinsburskiy <skinsbursky@parallels.com> >> >>> I think that's what we need to do: little patches that remove it from >>> one or another part of the code with careful explanation of why it >>> works. >>> >> >> Yes, thanks. I'll also try to simplify nfsd_open() by little patches. >> In general it looks like client_mutex protect two different things: >> open owner state (which is containerised already, actually) and >> files access. >> So, probably, this client mutex have to be converted into two: >> per-net one, which protects open owner state, and static one, which >> protects files operations. >> What do you think about it? > > That sounds right. > Looked into the code, and few more questions raised. So, currently, this mutex protects different NFS4 state structures and VFS file operations (like open, read, write, etc.). I would like this mutex to be converted into per-net one. Since NFS4 state structures are per-net already (with my previous patch set), the only thing left is VFS operations. But do we really need to protect them somehow from concurrent access? I.e. do we have some RPCs, which does more that one file manipulation, but this manipulations sequence have to be "atomic" in NFSd terms?
On Fri, Nov 23, 2012 at 03:31:11PM +0400, Stanislav Kinsbursky wrote: > 19.11.2012 16:46, bfields@fieldses.org ?????: > >On Mon, Nov 19, 2012 at 12:39:23PM +0400, Stanislav Kinsbursky wrote: > >>16.11.2012 20:58, bfields@fieldses.org ?????: > >>> > >>>A patch follows: note it's a two-line patch, with 20 lines of changelog > >>>showing that I looked at what state might be shared by other threads and > >>>explaining why I think this is safe. > >>> > >> > >>Acked-by: Stanislav Kinsburskiy <skinsbursky@parallels.com> > >> > >>>I think that's what we need to do: little patches that remove it from > >>>one or another part of the code with careful explanation of why it > >>>works. > >>> > >> > >>Yes, thanks. I'll also try to simplify nfsd_open() by little patches. > >>In general it looks like client_mutex protect two different things: > >>open owner state (which is containerised already, actually) and > >>files access. > >>So, probably, this client mutex have to be converted into two: > >>per-net one, which protects open owner state, and static one, which > >>protects files operations. > >>What do you think about it? > > > >That sounds right. > > > > Looked into the code, and few more questions raised. > So, currently, this mutex protects different NFS4 state structures and VFS file operations (like open, read, write, etc.). > I would like this mutex to be converted into per-net one. Since NFS4 > state structures are per-net already (with my previous patch set), > the only thing left is VFS operations. > But do we really need to protect them somehow from concurrent access? > I.e. do we have some RPCs, which does more that one file manipulation, but this manipulations sequence have to be "atomic" in NFSd terms? Probably not, but open is the most likely problem, as it does several things which are not all atomic from the point of view of the vfs. The mutex can't prevent races between an nfs client and a local user, but it can at least prevent races between nfs clients. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
27.11.2012 02:36, bfields@fieldses.org ?????: > On Fri, Nov 23, 2012 at 03:31:11PM +0400, Stanislav Kinsbursky wrote: >> 19.11.2012 16:46, bfields@fieldses.org ?????: >>> On Mon, Nov 19, 2012 at 12:39:23PM +0400, Stanislav Kinsbursky wrote: >>>> 16.11.2012 20:58, bfields@fieldses.org ?????: >>>>> >>>>> A patch follows: note it's a two-line patch, with 20 lines of changelog >>>>> showing that I looked at what state might be shared by other threads and >>>>> explaining why I think this is safe. >>>>> >>>> >>>> Acked-by: Stanislav Kinsburskiy <skinsbursky@parallels.com> >>>> >>>>> I think that's what we need to do: little patches that remove it from >>>>> one or another part of the code with careful explanation of why it >>>>> works. >>>>> >>>> >>>> Yes, thanks. I'll also try to simplify nfsd_open() by little patches. >>>> In general it looks like client_mutex protect two different things: >>>> open owner state (which is containerised already, actually) and >>>> files access. >>>> So, probably, this client mutex have to be converted into two: >>>> per-net one, which protects open owner state, and static one, which >>>> protects files operations. >>>> What do you think about it? >>> >>> That sounds right. >>> >> >> Looked into the code, and few more questions raised. >> So, currently, this mutex protects different NFS4 state structures and VFS file operations (like open, read, write, etc.). >> I would like this mutex to be converted into per-net one. Since NFS4 >> state structures are per-net already (with my previous patch set), >> the only thing left is VFS operations. >> But do we really need to protect them somehow from concurrent access? >> I.e. do we have some RPCs, which does more that one file manipulation, but this manipulations sequence have to be "atomic" in NFSd terms? > > Probably not, but open is the most likely problem, as it does several > things which are not all atomic from the point of view of the vfs. The > mutex can't prevent races between an nfs client and a local user, but it > can at least prevent races between nfs clients. > Ok then. You said, that you are going to optimize open a bit? I think client_mutex containerization can be delayed for a while. Thnaks. > --b. >
On Tue, Nov 27, 2012 at 11:54:30AM +0400, Stanislav Kinsbursky wrote: > 27.11.2012 02:36, bfields@fieldses.org ?????: > >On Fri, Nov 23, 2012 at 03:31:11PM +0400, Stanislav Kinsbursky wrote: > >>19.11.2012 16:46, bfields@fieldses.org ?????: > >>>On Mon, Nov 19, 2012 at 12:39:23PM +0400, Stanislav Kinsbursky wrote: > >>>>16.11.2012 20:58, bfields@fieldses.org ?????: > >>>>> > >>>>>A patch follows: note it's a two-line patch, with 20 lines of changelog > >>>>>showing that I looked at what state might be shared by other threads and > >>>>>explaining why I think this is safe. > >>>>> > >>>> > >>>>Acked-by: Stanislav Kinsburskiy <skinsbursky@parallels.com> > >>>> > >>>>>I think that's what we need to do: little patches that remove it from > >>>>>one or another part of the code with careful explanation of why it > >>>>>works. > >>>>> > >>>> > >>>>Yes, thanks. I'll also try to simplify nfsd_open() by little patches. > >>>>In general it looks like client_mutex protect two different things: > >>>>open owner state (which is containerised already, actually) and > >>>>files access. > >>>>So, probably, this client mutex have to be converted into two: > >>>>per-net one, which protects open owner state, and static one, which > >>>>protects files operations. > >>>>What do you think about it? > >>> > >>>That sounds right. > >>> > >> > >>Looked into the code, and few more questions raised. > >>So, currently, this mutex protects different NFS4 state structures and VFS file operations (like open, read, write, etc.). > >>I would like this mutex to be converted into per-net one. Since NFS4 > >>state structures are per-net already (with my previous patch set), > >>the only thing left is VFS operations. > >>But do we really need to protect them somehow from concurrent access? > >>I.e. do we have some RPCs, which does more that one file manipulation, but this manipulations sequence have to be "atomic" in NFSd terms? > > > >Probably not, but open is the most likely problem, as it does several > >things which are not all atomic from the point of view of the vfs. The > >mutex can't prevent races between an nfs client and a local user, but it > >can at least prevent races between nfs clients. > > > > Ok then. > You said, that you are going to optimize open a bit? It's on my todo list, but I'm not sure when it will be done. > I think client_mutex containerization can be delayed for a while. OK.--b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c index 43295d4..ad15e77 100644 --- a/fs/nfsd/nfs4recover.c +++ b/fs/nfsd/nfs4recover.c @@ -424,11 +424,9 @@ nfsd4_load_reboot_recovery_data(struct net *net) return -EINVAL; } - nfs4_lock_state(); status = nfsd4_init_recdir(); if (!status) status = nfsd4_recdir_load(); - nfs4_unlock_state(); if (status) printk(KERN_ERR "NFSD: Failure reading reboot recovery data\n"); return status;