* writeable file with no mnt_want_write()
@ 2008-07-03 12:21 Benny Halevy
2008-07-03 20:36 ` J. Bruce Fields
0 siblings, 1 reply; 9+ messages in thread
From: Benny Halevy @ 2008-07-03 12:21 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: NFS list
Bruce,
I'm seeing this warning on the open_downgrade path
when running the newpynfs tests:
Jul 3 07:32:50 buml kernel: writeable file with no mnt_want_write()
Jul 3 07:32:50 buml kernel: ------------[ cut here ]------------
Jul 3 07:32:50 buml kernel: WARNING: at /usr0/export/dev/bhalevy/git/linux-pnfs-bh-nfs41/include/linux/fs.h:855 drop_file_write_access+0x6b/0x7e()
Jul 3 07:32:50 buml kernel: Modules linked in: nfsd auth_rpcgss exportfs nfs lockd nfs_acl sunrpc
Jul 3 07:32:50 buml kernel: Call Trace:
Jul 3 07:32:50 buml kernel: 6eaadc88: [<6002f471>] warn_on_slowpath+0x54/0x8e
Jul 3 07:32:50 buml kernel: 6eaadcc8: [<601b790d>] printk+0xa0/0x793
Jul 3 07:32:50 buml kernel: 6eaadd38: [<601b6205>] __mutex_lock_slowpath+0x1db/0x1ea
Jul 3 07:32:50 buml kernel: 6eaadd68: [<7107d4d5>] nfs4_preprocess_seqid_op+0x2a6/0x31c [nfsd]
Jul 3 07:32:50 buml kernel: 6eaadda8: [<60078dc9>] drop_file_write_access+0x6b/0x7e
Jul 3 07:32:50 buml kernel: 6eaaddc8: [<710804e4>] nfsd4_open_downgrade+0x114/0x1de [nfsd]
Jul 3 07:32:50 buml kernel: 6eaade08: [<71076215>] nfsd4_proc_compound+0x1ba/0x2dc [nfsd]
Jul 3 07:32:50 buml kernel: 6eaade48: [<71068221>] nfsd_dispatch+0xe5/0x1c2 [nfsd]
Jul 3 07:32:50 buml kernel: 6eaade88: [<71312f81>] svc_process+0x3fd/0x714 [sunrpc]
Jul 3 07:32:50 buml kernel: 6eaadea8: [<60039a81>] kernel_sigprocmask+0xf3/0x100
Jul 3 07:32:50 buml kernel: 6eaadee8: [<7106874b>] nfsd+0x182/0x29b [nfsd]
Jul 3 07:32:50 buml kernel: 6eaadf48: [<60021cc9>] run_kernel_thread+0x41/0x4a
Jul 3 07:32:50 buml kernel: 6eaadf58: [<710685c9>] nfsd+0x0/0x29b [nfsd]
Jul 3 07:32:50 buml kernel: 6eaadf98: [<60021cb0>] run_kernel_thread+0x28/0x4a
Jul 3 07:32:50 buml kernel: 6eaadfc8: [<60013829>] new_thread_handler+0x72/0x9c
Jul 3 07:32:50 buml kernel:
Jul 3 07:32:50 buml kernel: ---[ end trace 2426dd7cb2fba3bf ]---
I'm not sure what would be the right fix for that...
The following could be unrelated, and maybe I'm just confused
but there seems to be something funky about the way we convert
from bmap to access bits.
(unrelated note: for nfsv4.1 we'll need to mask the share access with
~OPEN4_SHARE_ACCESS_WANT_DELEG_MASK)
If I understand this correctly, we set a bit in
st_access_bmap corresponding to the share_access,
so for read-only bit #1 will be, write-only #2,
and for read-write (only) bits 1-3 should be set.
otherwise this wouldn't work:
if (!test_bit(od->od_share_access, &stp->st_access_bmap)) {
dprintk("NFSD:access not a subset current bitmap: 0x%lx, input access=%08x\n",
stp->st_access_bmap, od->od_share_access);
goto out;
}
but init_stateid sets only one bit:
__set_bit(open->op_share_access, &stp->st_access_bmap);
only if we went through nfs4_upgrade_open another bit may be set.
Benny
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: writeable file with no mnt_want_write() 2008-07-03 12:21 writeable file with no mnt_want_write() Benny Halevy @ 2008-07-03 20:36 ` J. Bruce Fields 2008-07-04 12:34 ` Benny Halevy 0 siblings, 1 reply; 9+ messages in thread From: J. Bruce Fields @ 2008-07-03 20:36 UTC (permalink / raw) To: Benny Halevy; +Cc: NFS list On Thu, Jul 03, 2008 at 03:21:58PM +0300, Benny Halevy wrote: > Bruce, > > I'm seeing this warning on the open_downgrade path > when running the newpynfs tests: > > Jul 3 07:32:50 buml kernel: writeable file with no mnt_want_write() > Jul 3 07:32:50 buml kernel: ------------[ cut here ]------------ > Jul 3 07:32:50 buml kernel: WARNING: at /usr0/export/dev/bhalevy/git/linux-pnfs-bh-nfs41/include/linux/fs.h:855 drop_file_write_access+0x6b/0x7e() > Jul 3 07:32:50 buml kernel: Modules linked in: nfsd auth_rpcgss exportfs nfs lockd nfs_acl sunrpc > Jul 3 07:32:50 buml kernel: Call Trace: > Jul 3 07:32:50 buml kernel: 6eaadc88: [<6002f471>] warn_on_slowpath+0x54/0x8e > Jul 3 07:32:50 buml kernel: 6eaadcc8: [<601b790d>] printk+0xa0/0x793 > Jul 3 07:32:50 buml kernel: 6eaadd38: [<601b6205>] __mutex_lock_slowpath+0x1db/0x1ea > Jul 3 07:32:50 buml kernel: 6eaadd68: [<7107d4d5>] nfs4_preprocess_seqid_op+0x2a6/0x31c [nfsd] > Jul 3 07:32:50 buml kernel: 6eaadda8: [<60078dc9>] drop_file_write_access+0x6b/0x7e > Jul 3 07:32:50 buml kernel: 6eaaddc8: [<710804e4>] nfsd4_open_downgrade+0x114/0x1de [nfsd] > Jul 3 07:32:50 buml kernel: 6eaade08: [<71076215>] nfsd4_proc_compound+0x1ba/0x2dc [nfsd] > Jul 3 07:32:50 buml kernel: 6eaade48: [<71068221>] nfsd_dispatch+0xe5/0x1c2 [nfsd] > Jul 3 07:32:50 buml kernel: 6eaade88: [<71312f81>] svc_process+0x3fd/0x714 [sunrpc] > Jul 3 07:32:50 buml kernel: 6eaadea8: [<60039a81>] kernel_sigprocmask+0xf3/0x100 > Jul 3 07:32:50 buml kernel: 6eaadee8: [<7106874b>] nfsd+0x182/0x29b [nfsd] > Jul 3 07:32:50 buml kernel: 6eaadf48: [<60021cc9>] run_kernel_thread+0x41/0x4a > Jul 3 07:32:50 buml kernel: 6eaadf58: [<710685c9>] nfsd+0x0/0x29b [nfsd] > Jul 3 07:32:50 buml kernel: 6eaadf98: [<60021cb0>] run_kernel_thread+0x28/0x4a > Jul 3 07:32:50 buml kernel: 6eaadfc8: [<60013829>] new_thread_handler+0x72/0x9c > Jul 3 07:32:50 buml kernel: > Jul 3 07:32:50 buml kernel: ---[ end trace 2426dd7cb2fba3bf ]--- > > I'm not sure what would be the right fix for that... Yes. I'm a bit confused about that. Hm, maybe we need to be doing a mnt_want_write on open_upgrade and mnt_put_write on downgrade? > The following could be unrelated, and maybe I'm just confused > but there seems to be something funky about the way we convert > from bmap to access bits. > (unrelated note: for nfsv4.1 we'll need to mask the share access with > ~OPEN4_SHARE_ACCESS_WANT_DELEG_MASK) > > If I understand this correctly, we set a bit in > st_access_bmap corresponding to the share_access, > so for read-only bit #1 will be, write-only #2, > and for read-write (only) bits 1-3 should be set. No, in that case only the third bit 3 should be set. We're just trying to enforce rfc 3530 14.2.19: "The share_access and share_deny bits specified must be exactly equal to the union of the share_access and share_deny bits specified for some subset of the OPENs in effect for current openowner on the current file. If that constraint is not respected, the error NFS4ERR_INVAL should be returned." Note that this paragraph would allow OPEN for read OPEN for write OPEN_DOWNGRADE to read but have us return NFS4ERR_INVAL if we got a sequence of opens like: OPEN for read and write OPEN_DOWNGRADE to read because in this case there was only a single open (for read and write), and no open just for read. --b. > otherwise this wouldn't work: > if (!test_bit(od->od_share_access, &stp->st_access_bmap)) { > dprintk("NFSD:access not a subset current bitmap: 0x%lx, input access=%08x\n", > stp->st_access_bmap, od->od_share_access); > goto out; > } > > but init_stateid sets only one bit: > __set_bit(open->op_share_access, &stp->st_access_bmap); > > only if we went through nfs4_upgrade_open another bit may be set. > > Benny ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: writeable file with no mnt_want_write() 2008-07-03 20:36 ` J. Bruce Fields @ 2008-07-04 12:34 ` Benny Halevy 2008-07-04 12:38 ` [PATCH] nfsd: take file and mnt write in nfs4_upgrade_open Benny Halevy 2008-07-07 19:05 ` writeable file with no mnt_want_write() J. Bruce Fields 0 siblings, 2 replies; 9+ messages in thread From: Benny Halevy @ 2008-07-04 12:34 UTC (permalink / raw) To: J. Bruce Fields; +Cc: NFS list On Jul. 03, 2008, 23:36 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote: > On Thu, Jul 03, 2008 at 03:21:58PM +0300, Benny Halevy wrote: >> Bruce, >> >> I'm seeing this warning on the open_downgrade path >> when running the newpynfs tests: >> >> Jul 3 07:32:50 buml kernel: writeable file with no mnt_want_write() >> Jul 3 07:32:50 buml kernel: ------------[ cut here ]------------ >> Jul 3 07:32:50 buml kernel: WARNING: at /usr0/export/dev/bhalevy/git/linux-pnfs-bh-nfs41/include/linux/fs.h:855 drop_file_write_access+0x6b/0x7e() >> Jul 3 07:32:50 buml kernel: Modules linked in: nfsd auth_rpcgss exportfs nfs lockd nfs_acl sunrpc >> Jul 3 07:32:50 buml kernel: Call Trace: >> Jul 3 07:32:50 buml kernel: 6eaadc88: [<6002f471>] warn_on_slowpath+0x54/0x8e >> Jul 3 07:32:50 buml kernel: 6eaadcc8: [<601b790d>] printk+0xa0/0x793 >> Jul 3 07:32:50 buml kernel: 6eaadd38: [<601b6205>] __mutex_lock_slowpath+0x1db/0x1ea >> Jul 3 07:32:50 buml kernel: 6eaadd68: [<7107d4d5>] nfs4_preprocess_seqid_op+0x2a6/0x31c [nfsd] >> Jul 3 07:32:50 buml kernel: 6eaadda8: [<60078dc9>] drop_file_write_access+0x6b/0x7e >> Jul 3 07:32:50 buml kernel: 6eaaddc8: [<710804e4>] nfsd4_open_downgrade+0x114/0x1de [nfsd] >> Jul 3 07:32:50 buml kernel: 6eaade08: [<71076215>] nfsd4_proc_compound+0x1ba/0x2dc [nfsd] >> Jul 3 07:32:50 buml kernel: 6eaade48: [<71068221>] nfsd_dispatch+0xe5/0x1c2 [nfsd] >> Jul 3 07:32:50 buml kernel: 6eaade88: [<71312f81>] svc_process+0x3fd/0x714 [sunrpc] >> Jul 3 07:32:50 buml kernel: 6eaadea8: [<60039a81>] kernel_sigprocmask+0xf3/0x100 >> Jul 3 07:32:50 buml kernel: 6eaadee8: [<7106874b>] nfsd+0x182/0x29b [nfsd] >> Jul 3 07:32:50 buml kernel: 6eaadf48: [<60021cc9>] run_kernel_thread+0x41/0x4a >> Jul 3 07:32:50 buml kernel: 6eaadf58: [<710685c9>] nfsd+0x0/0x29b [nfsd] >> Jul 3 07:32:50 buml kernel: 6eaadf98: [<60021cb0>] run_kernel_thread+0x28/0x4a >> Jul 3 07:32:50 buml kernel: 6eaadfc8: [<60013829>] new_thread_handler+0x72/0x9c >> Jul 3 07:32:50 buml kernel: >> Jul 3 07:32:50 buml kernel: ---[ end trace 2426dd7cb2fba3bf ]--- >> >> I'm not sure what would be the right fix for that... > > Yes. I'm a bit confused about that. Hm, maybe we need to be doing a > mnt_want_write on open_upgrade and mnt_put_write on downgrade? Right on (at least for the first part of your suggestion :) See patch in reply to this message that makes this warning go away. > >> The following could be unrelated, and maybe I'm just confused >> but there seems to be something funky about the way we convert >> from bmap to access bits. >> (unrelated note: for nfsv4.1 we'll need to mask the share access with >> ~OPEN4_SHARE_ACCESS_WANT_DELEG_MASK) >> >> If I understand this correctly, we set a bit in >> st_access_bmap corresponding to the share_access, >> so for read-only bit #1 will be, write-only #2, >> and for read-write (only) bits 1-3 should be set. > > No, in that case only the third bit 3 should be set. > > We're just trying to enforce rfc 3530 14.2.19: OK. I see. Thanks for explaining! It is a bit mind boggling, maybe adding some comments explaining why the bitmap is needed would help... Benny > > "The share_access and share_deny bits specified must be exactly > equal to the union of the share_access and share_deny bits > specified for some subset of the OPENs in effect for current > openowner on the current file. If that constraint is not > respected, the error NFS4ERR_INVAL should be returned." > > Note that this paragraph would allow > > OPEN for read > OPEN for write > OPEN_DOWNGRADE to read > > > but have us return NFS4ERR_INVAL if we got a sequence of opens like: > > OPEN for read and write > OPEN_DOWNGRADE to read > > because in this case there was only a single open (for read and write), > and no open just for read. > > --b. > >> otherwise this wouldn't work: >> if (!test_bit(od->od_share_access, &stp->st_access_bmap)) { >> dprintk("NFSD:access not a subset current bitmap: 0x%lx, input access=%08x\n", >> stp->st_access_bmap, od->od_share_access); >> goto out; >> } >> >> but init_stateid sets only one bit: >> __set_bit(open->op_share_access, &stp->st_access_bmap); >> >> only if we went through nfs4_upgrade_open another bit may be set. >> >> Benny ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] nfsd: take file and mnt write in nfs4_upgrade_open 2008-07-04 12:34 ` Benny Halevy @ 2008-07-04 12:38 ` Benny Halevy 2008-07-07 19:25 ` J. Bruce Fields 2008-07-07 19:05 ` writeable file with no mnt_want_write() J. Bruce Fields 1 sibling, 1 reply; 9+ messages in thread From: Benny Halevy @ 2008-07-04 12:38 UTC (permalink / raw) To: J. Bruce Fields; +Cc: linux-nfs, Benny Halevy testing with newpynfs revealed this warning: Jul 3 07:32:50 buml kernel: writeable file with no mnt_want_write() Jul 3 07:32:50 buml kernel: ------------[ cut here ]------------ Jul 3 07:32:50 buml kernel: WARNING: at /usr0/export/dev/bhalevy/git/linux-pnfs-bh-nfs41/include/linux/fs.h:855 drop_file_write_access+0x6b/0x7e() Jul 3 07:32:50 buml kernel: Modules linked in: nfsd auth_rpcgss exportfs nfs lockd nfs_acl sunrpc Jul 3 07:32:50 buml kernel: Call Trace: Jul 3 07:32:50 buml kernel: 6eaadc88: [<6002f471>] warn_on_slowpath+0x54/0x8e Jul 3 07:32:50 buml kernel: 6eaadcc8: [<601b790d>] printk+0xa0/0x793 Jul 3 07:32:50 buml kernel: 6eaadd38: [<601b6205>] __mutex_lock_slowpath+0x1db/0x1ea Jul 3 07:32:50 buml kernel: 6eaadd68: [<7107d4d5>] nfs4_preprocess_seqid_op+0x2a6/0x31c [nfsd] Jul 3 07:32:50 buml kernel: 6eaadda8: [<60078dc9>] drop_file_write_access+0x6b/0x7e Jul 3 07:32:50 buml kernel: 6eaaddc8: [<710804e4>] nfsd4_open_downgrade+0x114/0x1de [nfsd] Jul 3 07:32:50 buml kernel: 6eaade08: [<71076215>] nfsd4_proc_compound+0x1ba/0x2dc [nfsd] Jul 3 07:32:50 buml kernel: 6eaade48: [<71068221>] nfsd_dispatch+0xe5/0x1c2 [nfsd] Jul 3 07:32:50 buml kernel: 6eaade88: [<71312f81>] svc_process+0x3fd/0x714 [sunrpc] Jul 3 07:32:50 buml kernel: 6eaadea8: [<60039a81>] kernel_sigprocmask+0xf3/0x100 Jul 3 07:32:50 buml kernel: 6eaadee8: [<7106874b>] nfsd+0x182/0x29b [nfsd] Jul 3 07:32:50 buml kernel: 6eaadf48: [<60021cc9>] run_kernel_thread+0x41/0x4a Jul 3 07:32:50 buml kernel: 6eaadf58: [<710685c9>] nfsd+0x0/0x29b [nfsd] Jul 3 07:32:50 buml kernel: 6eaadf98: [<60021cb0>] run_kernel_thread+0x28/0x4a Jul 3 07:32:50 buml kernel: 6eaadfc8: [<60013829>] new_thread_handler+0x72/0x9c Jul 3 07:32:50 buml kernel: Jul 3 07:32:50 buml kernel: ---[ end trace 2426dd7cb2fba3bf ]--- Bruce Fields suggested this (Thanks!): maybe we need to be doing a mnt_want_write on open_upgrade and mnt_put_write on downgrade? This patch adds a call to mnt_want_write and file_take_write (which is doing the actual work). The counter-calls mnt_drop_write a file_release_write are now being properly called by drop_file_write_access in the exact path printed by the warning above. Signed-off-by: Benny Halevy <bhalevy@panasas.com> --- fs/nfsd/nfs4state.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 0d1760f..4263445 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -1570,6 +1570,10 @@ nfs4_upgrade_open(struct svc_rqst *rqstp, struct svc_fh *cur_fh, struct nfs4_sta int err = get_write_access(inode); if (err) return nfserrno(err); + err = mnt_want_write(cur_fh->fh_export->ex_path.mnt); + if (err) + return nfserrno(err); + file_take_write(filp); } status = nfsd4_truncate(rqstp, cur_fh, open); if (status) { -- 1.5.6.GIT ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] nfsd: take file and mnt write in nfs4_upgrade_open 2008-07-04 12:38 ` [PATCH] nfsd: take file and mnt write in nfs4_upgrade_open Benny Halevy @ 2008-07-07 19:25 ` J. Bruce Fields 2008-07-08 8:16 ` Benny Halevy 0 siblings, 1 reply; 9+ messages in thread From: J. Bruce Fields @ 2008-07-07 19:25 UTC (permalink / raw) To: Benny Halevy; +Cc: linux-nfs Yes, this looks correct, at least for the immediate fix; thanks! Eventually I think we should move to opening a new file descriptor when upgrading, and keeping two file descriptors with the stateid; I think this business of trying to use a file descriptor for write when we opened it for read is probably wrong. --b. On Fri, Jul 04, 2008 at 03:38:41PM +0300, Benny Halevy wrote: > testing with newpynfs revealed this warning: > Jul 3 07:32:50 buml kernel: writeable file with no mnt_want_write() > Jul 3 07:32:50 buml kernel: ------------[ cut here ]------------ > Jul 3 07:32:50 buml kernel: WARNING: at /usr0/export/dev/bhalevy/git/linux-pnfs-bh-nfs41/include/linux/fs.h:855 drop_file_write_access+0x6b/0x7e() > Jul 3 07:32:50 buml kernel: Modules linked in: nfsd auth_rpcgss exportfs nfs lockd nfs_acl sunrpc > Jul 3 07:32:50 buml kernel: Call Trace: > Jul 3 07:32:50 buml kernel: 6eaadc88: [<6002f471>] warn_on_slowpath+0x54/0x8e > Jul 3 07:32:50 buml kernel: 6eaadcc8: [<601b790d>] printk+0xa0/0x793 > Jul 3 07:32:50 buml kernel: 6eaadd38: [<601b6205>] __mutex_lock_slowpath+0x1db/0x1ea > Jul 3 07:32:50 buml kernel: 6eaadd68: [<7107d4d5>] nfs4_preprocess_seqid_op+0x2a6/0x31c [nfsd] > Jul 3 07:32:50 buml kernel: 6eaadda8: [<60078dc9>] drop_file_write_access+0x6b/0x7e > Jul 3 07:32:50 buml kernel: 6eaaddc8: [<710804e4>] nfsd4_open_downgrade+0x114/0x1de [nfsd] > Jul 3 07:32:50 buml kernel: 6eaade08: [<71076215>] nfsd4_proc_compound+0x1ba/0x2dc [nfsd] > Jul 3 07:32:50 buml kernel: 6eaade48: [<71068221>] nfsd_dispatch+0xe5/0x1c2 [nfsd] > Jul 3 07:32:50 buml kernel: 6eaade88: [<71312f81>] svc_process+0x3fd/0x714 [sunrpc] > Jul 3 07:32:50 buml kernel: 6eaadea8: [<60039a81>] kernel_sigprocmask+0xf3/0x100 > Jul 3 07:32:50 buml kernel: 6eaadee8: [<7106874b>] nfsd+0x182/0x29b [nfsd] > Jul 3 07:32:50 buml kernel: 6eaadf48: [<60021cc9>] run_kernel_thread+0x41/0x4a > Jul 3 07:32:50 buml kernel: 6eaadf58: [<710685c9>] nfsd+0x0/0x29b [nfsd] > Jul 3 07:32:50 buml kernel: 6eaadf98: [<60021cb0>] run_kernel_thread+0x28/0x4a > Jul 3 07:32:50 buml kernel: 6eaadfc8: [<60013829>] new_thread_handler+0x72/0x9c > Jul 3 07:32:50 buml kernel: > Jul 3 07:32:50 buml kernel: ---[ end trace 2426dd7cb2fba3bf ]--- > > Bruce Fields suggested this (Thanks!): > maybe we need to be doing a mnt_want_write on open_upgrade and mnt_put_write on downgrade? > > This patch adds a call to mnt_want_write and file_take_write (which is > doing the actual work). > > The counter-calls mnt_drop_write a file_release_write are now being properly > called by drop_file_write_access in the exact path printed by the warning > above. > > Signed-off-by: Benny Halevy <bhalevy@panasas.com> > --- > fs/nfsd/nfs4state.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 0d1760f..4263445 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -1570,6 +1570,10 @@ nfs4_upgrade_open(struct svc_rqst *rqstp, struct svc_fh *cur_fh, struct nfs4_sta > int err = get_write_access(inode); > if (err) > return nfserrno(err); > + err = mnt_want_write(cur_fh->fh_export->ex_path.mnt); > + if (err) > + return nfserrno(err); > + file_take_write(filp); > } > status = nfsd4_truncate(rqstp, cur_fh, open); > if (status) { > -- > 1.5.6.GIT > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nfsd: take file and mnt write in nfs4_upgrade_open 2008-07-07 19:25 ` J. Bruce Fields @ 2008-07-08 8:16 ` Benny Halevy 2008-07-08 14:35 ` J. Bruce Fields 0 siblings, 1 reply; 9+ messages in thread From: Benny Halevy @ 2008-07-08 8:16 UTC (permalink / raw) To: J. Bruce Fields; +Cc: linux-nfs On Jul. 07, 2008, 22:25 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote: > Yes, this looks correct, at least for the immediate fix; thanks! > > Eventually I think we should move to opening a new file descriptor when > upgrading, and keeping two file descriptors with the stateid; I think > this business of trying to use a file descriptor for write when we > opened it for read is probably wrong. That could work, though they need to be associated in some way for open_downgrade/close processing. Benny > > --b. > > On Fri, Jul 04, 2008 at 03:38:41PM +0300, Benny Halevy wrote: >> testing with newpynfs revealed this warning: >> Jul 3 07:32:50 buml kernel: writeable file with no mnt_want_write() >> Jul 3 07:32:50 buml kernel: ------------[ cut here ]------------ >> Jul 3 07:32:50 buml kernel: WARNING: at /usr0/export/dev/bhalevy/git/linux-pnfs-bh-nfs41/include/linux/fs.h:855 drop_file_write_access+0x6b/0x7e() >> Jul 3 07:32:50 buml kernel: Modules linked in: nfsd auth_rpcgss exportfs nfs lockd nfs_acl sunrpc >> Jul 3 07:32:50 buml kernel: Call Trace: >> Jul 3 07:32:50 buml kernel: 6eaadc88: [<6002f471>] warn_on_slowpath+0x54/0x8e >> Jul 3 07:32:50 buml kernel: 6eaadcc8: [<601b790d>] printk+0xa0/0x793 >> Jul 3 07:32:50 buml kernel: 6eaadd38: [<601b6205>] __mutex_lock_slowpath+0x1db/0x1ea >> Jul 3 07:32:50 buml kernel: 6eaadd68: [<7107d4d5>] nfs4_preprocess_seqid_op+0x2a6/0x31c [nfsd] >> Jul 3 07:32:50 buml kernel: 6eaadda8: [<60078dc9>] drop_file_write_access+0x6b/0x7e >> Jul 3 07:32:50 buml kernel: 6eaaddc8: [<710804e4>] nfsd4_open_downgrade+0x114/0x1de [nfsd] >> Jul 3 07:32:50 buml kernel: 6eaade08: [<71076215>] nfsd4_proc_compound+0x1ba/0x2dc [nfsd] >> Jul 3 07:32:50 buml kernel: 6eaade48: [<71068221>] nfsd_dispatch+0xe5/0x1c2 [nfsd] >> Jul 3 07:32:50 buml kernel: 6eaade88: [<71312f81>] svc_process+0x3fd/0x714 [sunrpc] >> Jul 3 07:32:50 buml kernel: 6eaadea8: [<60039a81>] kernel_sigprocmask+0xf3/0x100 >> Jul 3 07:32:50 buml kernel: 6eaadee8: [<7106874b>] nfsd+0x182/0x29b [nfsd] >> Jul 3 07:32:50 buml kernel: 6eaadf48: [<60021cc9>] run_kernel_thread+0x41/0x4a >> Jul 3 07:32:50 buml kernel: 6eaadf58: [<710685c9>] nfsd+0x0/0x29b [nfsd] >> Jul 3 07:32:50 buml kernel: 6eaadf98: [<60021cb0>] run_kernel_thread+0x28/0x4a >> Jul 3 07:32:50 buml kernel: 6eaadfc8: [<60013829>] new_thread_handler+0x72/0x9c >> Jul 3 07:32:50 buml kernel: >> Jul 3 07:32:50 buml kernel: ---[ end trace 2426dd7cb2fba3bf ]--- >> >> Bruce Fields suggested this (Thanks!): >> maybe we need to be doing a mnt_want_write on open_upgrade and mnt_put_write on downgrade? >> >> This patch adds a call to mnt_want_write and file_take_write (which is >> doing the actual work). >> >> The counter-calls mnt_drop_write a file_release_write are now being properly >> called by drop_file_write_access in the exact path printed by the warning >> above. >> >> Signed-off-by: Benny Halevy <bhalevy@panasas.com> >> --- >> fs/nfsd/nfs4state.c | 4 ++++ >> 1 files changed, 4 insertions(+), 0 deletions(-) >> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c >> index 0d1760f..4263445 100644 >> --- a/fs/nfsd/nfs4state.c >> +++ b/fs/nfsd/nfs4state.c >> @@ -1570,6 +1570,10 @@ nfs4_upgrade_open(struct svc_rqst *rqstp, struct svc_fh *cur_fh, struct nfs4_sta >> int err = get_write_access(inode); >> if (err) >> return nfserrno(err); >> + err = mnt_want_write(cur_fh->fh_export->ex_path.mnt); >> + if (err) >> + return nfserrno(err); >> + file_take_write(filp); >> } >> status = nfsd4_truncate(rqstp, cur_fh, open); >> if (status) { >> -- >> 1.5.6.GIT >> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nfsd: take file and mnt write in nfs4_upgrade_open 2008-07-08 8:16 ` Benny Halevy @ 2008-07-08 14:35 ` J. Bruce Fields 0 siblings, 0 replies; 9+ messages in thread From: J. Bruce Fields @ 2008-07-08 14:35 UTC (permalink / raw) To: Benny Halevy; +Cc: linux-nfs On Tue, Jul 08, 2008 at 11:16:49AM +0300, Benny Halevy wrote: > On Jul. 07, 2008, 22:25 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote: > > Yes, this looks correct, at least for the immediate fix; thanks! > > > > Eventually I think we should move to opening a new file descriptor when > > upgrading, and keeping two file descriptors with the stateid; I think > > this business of trying to use a file descriptor for write when we > > opened it for read is probably wrong. > > That could work, though they need to be associated in some way > for open_downgrade/close processing. Yes. I think it would work just to keep up to one read-only open and one write-only open and then close one or the other as necessary on the downgrade. --b. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: writeable file with no mnt_want_write() 2008-07-04 12:34 ` Benny Halevy 2008-07-04 12:38 ` [PATCH] nfsd: take file and mnt write in nfs4_upgrade_open Benny Halevy @ 2008-07-07 19:05 ` J. Bruce Fields 2008-07-08 8:40 ` Benny Halevy 1 sibling, 1 reply; 9+ messages in thread From: J. Bruce Fields @ 2008-07-07 19:05 UTC (permalink / raw) To: Benny Halevy; +Cc: NFS list On Fri, Jul 04, 2008 at 03:34:11PM +0300, Benny Halevy wrote: > On Jul. 03, 2008, 23:36 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote: > > We're just trying to enforce rfc 3530 14.2.19: > > OK. I see. > Thanks for explaining! > It is a bit mind boggling, maybe adding some comments explaining > why the bitmap is needed would help... Where do you think you would have looked for a comment? I figured just before the helper functions here was one obvious place. --b. commit 4f83aa302f8f8b42397c6d3703d670f0588c03ec Author: J. Bruce Fields <bfields@citi.umich.edu> Date: Mon Jul 7 15:02:02 2008 -0400 nfsd: document open share bit tracking It's not immediately obvious from the code why we're doing this. Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu> Cc: Benny Halevy <bhalevy@panasas.com> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index eca8aaa..c29b6ed 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -1173,6 +1173,24 @@ static inline int deny_valid(u32 x) return x <= NFS4_SHARE_DENY_BOTH; } +/* + * We store the NONE, READ, WRITE, and BOTH bits separately in the + * st_{access,deny}_bmap field of the stateid, in order to track not + * only what share bits are currently in force, but also what + * combinations of share bits previous opens have used. This allows us + * to enforce the recommendation of rfc 3530 14.2.19 that the server + * return an error if the client attempt to downgrade to a combination + * of share bits not explicable by closing some of its previous opens. + * + * XXX: This enforcement is actually incomplete, since we don't keep + * track of access/deny bit combinations; so, e.g., we allow: + * + * OPEN allow read, deny write + * OPEN allow both, deny none + * DOWNGRADE allow read, deny none + * + * which we should reject. + */ static void set_access(unsigned int *access, unsigned long bmap) { int i; ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: writeable file with no mnt_want_write() 2008-07-07 19:05 ` writeable file with no mnt_want_write() J. Bruce Fields @ 2008-07-08 8:40 ` Benny Halevy 0 siblings, 0 replies; 9+ messages in thread From: Benny Halevy @ 2008-07-08 8:40 UTC (permalink / raw) To: J. Bruce Fields; +Cc: NFS list On Jul. 07, 2008, 22:05 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote: > On Fri, Jul 04, 2008 at 03:34:11PM +0300, Benny Halevy wrote: >> On Jul. 03, 2008, 23:36 +0300, "J. Bruce Fields" <bfields@fieldses.org> wrote: >>> We're just trying to enforce rfc 3530 14.2.19: >> OK. I see. >> Thanks for explaining! >> It is a bit mind boggling, maybe adding some comments explaining >> why the bitmap is needed would help... > > Where do you think you would have looked for a comment? I figured just > before the helper functions here was one obvious place. Yup. Either here, or closer to (nfsd's) struct nfs4_stateid's definition where st_{access,deny}_bmap are defined. Benny > > --b. > > commit 4f83aa302f8f8b42397c6d3703d670f0588c03ec > Author: J. Bruce Fields <bfields@citi.umich.edu> > Date: Mon Jul 7 15:02:02 2008 -0400 > > nfsd: document open share bit tracking > > It's not immediately obvious from the code why we're doing this. > > Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu> > Cc: Benny Halevy <bhalevy@panasas.com> > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index eca8aaa..c29b6ed 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -1173,6 +1173,24 @@ static inline int deny_valid(u32 x) > return x <= NFS4_SHARE_DENY_BOTH; > } > > +/* > + * We store the NONE, READ, WRITE, and BOTH bits separately in the > + * st_{access,deny}_bmap field of the stateid, in order to track not > + * only what share bits are currently in force, but also what > + * combinations of share bits previous opens have used. This allows us > + * to enforce the recommendation of rfc 3530 14.2.19 that the server > + * return an error if the client attempt to downgrade to a combination > + * of share bits not explicable by closing some of its previous opens. > + * > + * XXX: This enforcement is actually incomplete, since we don't keep > + * track of access/deny bit combinations; so, e.g., we allow: > + * > + * OPEN allow read, deny write > + * OPEN allow both, deny none > + * DOWNGRADE allow read, deny none > + * > + * which we should reject. > + */ > static void > set_access(unsigned int *access, unsigned long bmap) { > int i; ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2008-07-08 14:35 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-07-03 12:21 writeable file with no mnt_want_write() Benny Halevy 2008-07-03 20:36 ` J. Bruce Fields 2008-07-04 12:34 ` Benny Halevy 2008-07-04 12:38 ` [PATCH] nfsd: take file and mnt write in nfs4_upgrade_open Benny Halevy 2008-07-07 19:25 ` J. Bruce Fields 2008-07-08 8:16 ` Benny Halevy 2008-07-08 14:35 ` J. Bruce Fields 2008-07-07 19:05 ` writeable file with no mnt_want_write() J. Bruce Fields 2008-07-08 8:40 ` Benny Halevy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox