| Message ID | 20110727110259.204979.56782.stgit@localhost.localdomain |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 27, 2011 at 01:02:59PM +0200, Bernd Schubert wrote: > Use 32-bit or 64-bit llseek() hashes for directory offsets depending on > the NFS version. NFSv2 gets 32-bit hashes only. Independent of the O_ vs FMODE thing make sure you pass the correct flag at open time, instead of racy runtime modifications. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/27/2011 11:03 PM, Christoph Hellwig wrote: > On Wed, Jul 27, 2011 at 01:02:59PM +0200, Bernd Schubert wrote: >> Use 32-bit or 64-bit llseek() hashes for directory offsets depending on >> the NFS version. NFSv2 gets 32-bit hashes only. > > Independent of the O_ vs FMODE thing make sure you pass the correct > flag at open time, instead of racy runtime modifications. > Christoph, before I'm going to work further on the patch sets, I have few questions first. Could you please help me with that? file->f_mode is set in __dentry_open() based on O_ flags. So if f_mode is supposed to be set directly during the NFS open call we would need O_ *and* FMODE flags, Now I still do not understand why we cannot add a flag *after* the open call and only in nfsd_readdir()? I do not see any races there. nfsd_readdir() gets its 'struct file' from get_empty_filp() and __dentry_open(). Now as 'struct file' is allocated by get_empty_filp() it also cannot be used by any other thread. nfsd_readdir() just reads the directory and closes it immedeatily after the readdir(). So where is there supposed to be a race? And lastly, if we are going to set f_mode directly at open time, we have the choice to 1) Specify those new O_ flags for all files. While setting a flag is a cheap operation, it still wastes CPU cycles for file opens, as files do not need that flag. 2) Duplicate nfsd_open() to implement a new function for directories only. I think not a good idea either. 3) Rewrite the nfsd_open() function to allow to set flags from calling functions. That would mean to update the nfsd code at at least 8 places. Do we really want to go that way? So altogether, updating the patches to replace O_ by FMODE flags is easy, but setting that flag in nfsd at open time, would mean a large overhead. Thanks, Bernd -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 28, 2011 at 11:19:07AM +0200, Bernd Schubert wrote: > before I'm going to work further on the patch sets, I have few > questions first. Could you please help me with that? > > file->f_mode is set in __dentry_open() based on O_ flags. Or right after dentry_open, before anyone can possibly grab a reference to the file. Just do that in nfsd_open based on an NFSD_MAY_ flag. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" 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/vfs.c b/fs/nfsd/vfs.c index fd0acca..d79bbcd 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -1994,6 +1994,12 @@ nfsd_readdir(struct svc_rqst *rqstp, struct svc_fh *fhp, loff_t *offsetp, if (err) goto out; + /* NFSv2 only supports 32 bit cookies */ + if (rqstp->rq_vers > 2) + file->f_flags &= O_64BITHASH; + else + file->f_flags &= O_32BITHASH; + offset = vfs_llseek(file, offset, 0); if (offset < 0) { err = nfserrno((int)offset);
Use 32-bit or 64-bit llseek() hashes for directory offsets depending on the NFS version. NFSv2 gets 32-bit hashes only. Signed-off-by: Bernd Schubert <bernd.schubert@itwm.fraunhofer.de> --- fs/nfsd/vfs.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html