| Message ID | 960d206f-3cb5-b60e-5245-d7282dabf664@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Dec 31, 2016 at 09:18:08PM +0800, Kinglong Mee wrote: > Commit fae5096ad217 > "nfsd: assume writeable exportabled filesystems have f_sync" > have remove the checking of f_sync. > > Christoph Hellwig suggests, > "Warn and refuse the writable export." > > I think just covert to a readonly export for !fsync filesystem, > also, for a readonly filesystem is reasonable. Hmmm. It's not something we've done before. Off hand, I can't see why it would cause a problem, but I'm not convinced yet. Could you add to the changelog a description of the use case you gave Christoph in your defense of this idea? Also: > Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> > --- > fs/nfsd/export.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c > index 43e109c..3ec3b6b 100644 > --- a/fs/nfsd/export.c > +++ b/fs/nfsd/export.c > @@ -358,6 +358,18 @@ static int check_export(struct inode *inode, int *flags, unsigned char *uuid) > if (*flags & NFSEXP_V4ROOT) > *flags |= NFSEXP_READONLY; > > + /* > + * Convert to a readonly export for that, > + * 1. not supported fsync filesystem, > + * 2. readonly filesystem. > + */ > + if ((!inode->i_fop->fsync || IS_RDONLY(inode)) > + && !(*flags & NFSEXP_READONLY)) { > + dprintk("exp_export: Only support readonly export " > + "for fsync unsupported or readonly filesystem.\n"); Something like this might be more helpful: "Filesystem %s: exporting read-only\n", IS_RDONLY(inode) ? "is read-only" : "has no fsync method" Also if we passed the dentry to check_export, could we do something like: "%s %s: exporting read-only\n", d_path(dentry,...), IS_RDONLY... here and in the other warnings? --b. > + *flags |= NFSEXP_READONLY; > + } > + > /* There are two requirements on a filesystem to be exportable. > * 1: We must be able to identify the filesystem from a number. > * either a device number (so FS_REQUIRES_DEV needed) > -- > 2.9.3 -- 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
On Thu, Jan 5, 2017 at 1:29 AM, J. Bruce Fields <bfields@fieldses.org> wrote: > On Sat, Dec 31, 2016 at 09:18:08PM +0800, Kinglong Mee wrote: >> Commit fae5096ad217 >> "nfsd: assume writeable exportabled filesystems have f_sync" >> have remove the checking of f_sync. >> >> Christoph Hellwig suggests, >> "Warn and refuse the writable export." >> >> I think just covert to a readonly export for !fsync filesystem, >> also, for a readonly filesystem is reasonable. > > Hmmm. It's not something we've done before. Off hand, I can't see why > it would cause a problem, but I'm not convinced yet. > > Could you add to the changelog a description of the use case you gave > Christoph in your defense of this idea? Okay, I will give more description about the patch include that. > > Also: > >> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> >> --- >> fs/nfsd/export.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c >> index 43e109c..3ec3b6b 100644 >> --- a/fs/nfsd/export.c >> +++ b/fs/nfsd/export.c >> @@ -358,6 +358,18 @@ static int check_export(struct inode *inode, int *flags, unsigned char *uuid) >> if (*flags & NFSEXP_V4ROOT) >> *flags |= NFSEXP_READONLY; >> >> + /* >> + * Convert to a readonly export for that, >> + * 1. not supported fsync filesystem, >> + * 2. readonly filesystem. >> + */ >> + if ((!inode->i_fop->fsync || IS_RDONLY(inode)) >> + && !(*flags & NFSEXP_READONLY)) { >> + dprintk("exp_export: Only support readonly export " >> + "for fsync unsupported or readonly filesystem.\n"); > > Something like this might be more helpful: > > "Filesystem %s: exporting read-only\n", IS_RDONLY(inode) ? > "is read-only" : "has no fsync method" > > Also if we passed the dentry to check_export, could we do something > like: > > "%s %s: exporting read-only\n", d_path(dentry,...), IS_RDONLY... > > here and in the other warnings? A kstrdup from svc_export_parse() 's string path parsing is simplify, also, I will show in the next patch. thanks, Kinglong Mee > > --b. > >> + *flags |= NFSEXP_READONLY; >> + } >> + >> /* There are two requirements on a filesystem to be exportable. >> * 1: We must be able to identify the filesystem from a number. >> * either a device number (so FS_REQUIRES_DEV needed) >> -- >> 2.9.3 -- 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
On Sat, Dec 31, 2016 at 09:18:08PM +0800, Kinglong Mee wrote: > Commit fae5096ad217 > "nfsd: assume writeable exportabled filesystems have f_sync" > have remove the checking of f_sync. > > Christoph Hellwig suggests, > "Warn and refuse the writable export." > > I think just covert to a readonly export for !fsync filesystem, > also, for a readonly filesystem is reasonable. I don't like degrading the export. We should require an explicit ro option in this case. -- 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
On 1/8/2017 18:07, Christoph Hellwig wrote: > On Sat, Dec 31, 2016 at 09:18:08PM +0800, Kinglong Mee wrote: >> Commit fae5096ad217 >> "nfsd: assume writeable exportabled filesystems have f_sync" >> have remove the checking of f_sync. >> >> Christoph Hellwig suggests, >> "Warn and refuse the writable export." >> >> I think just covert to a readonly export for !fsync filesystem, >> also, for a readonly filesystem is reasonable. > > I don't like degrading the export. We should require an explicit > ro option in this case. With this patch, we can see the ro option in the proc file. # mount |grep xfs /dev/sdc on /nfs type xfs (ro,relatime,seclabel,attr2,inode64,noquota) # cat /etc/exports /nfs/ *(rw,no_subtree_check,no_root_squash,insecure,fsid=0) # cat /proc/fs/nfsd/exports # Version 1.1 # Path Client(Flags) # IPs /nfs *(ro,insecure,no_root_squash,sync,wdelay,no_subtree_check,fsid=0,uuid=a4a352bc:252a47cb:b3953193:040e050d,sec=1,rw,insecure,no_root_squash) thanks, Kinglong Mee -- 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
On Sun, Jan 08, 2017 at 02:07:15AM -0800, Christoph Hellwig wrote: > On Sat, Dec 31, 2016 at 09:18:08PM +0800, Kinglong Mee wrote: > > Commit fae5096ad217 > > "nfsd: assume writeable exportabled filesystems have f_sync" > > have remove the checking of f_sync. > > > > Christoph Hellwig suggests, > > "Warn and refuse the writable export." > > > > I think just covert to a readonly export for !fsync filesystem, > > also, for a readonly filesystem is reasonable. > > I don't like degrading the export. Anything there other than an intuition? > We should require an explicit ro option in this case. Well, I can't tell if Kinglong's case is something people are actively complaining about or more hypotethetical, and in any case it doesn't seem like a big deal, so I'm ignoring this for now, I guess.... --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/export.c b/fs/nfsd/export.c index 43e109c..3ec3b6b 100644 --- a/fs/nfsd/export.c +++ b/fs/nfsd/export.c @@ -358,6 +358,18 @@ static int check_export(struct inode *inode, int *flags, unsigned char *uuid) if (*flags & NFSEXP_V4ROOT) *flags |= NFSEXP_READONLY; + /* + * Convert to a readonly export for that, + * 1. not supported fsync filesystem, + * 2. readonly filesystem. + */ + if ((!inode->i_fop->fsync || IS_RDONLY(inode)) + && !(*flags & NFSEXP_READONLY)) { + dprintk("exp_export: Only support readonly export " + "for fsync unsupported or readonly filesystem.\n"); + *flags |= NFSEXP_READONLY; + } + /* There are two requirements on a filesystem to be exportable. * 1: We must be able to identify the filesystem from a number. * either a device number (so FS_REQUIRES_DEV needed)
Commit fae5096ad217 "nfsd: assume writeable exportabled filesystems have f_sync" have remove the checking of f_sync. Christoph Hellwig suggests, "Warn and refuse the writable export." I think just covert to a readonly export for !fsync filesystem, also, for a readonly filesystem is reasonable. Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> --- fs/nfsd/export.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)