[PATCH] cifs: Allow nfsd over cifs

[PATCH] cifs: Allow nfsd over cifs

[shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w]

From: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>


Define structure cifs_fid to span 64 bits of inode ids.
Allow nfsd over cifs.
Add export ops encode_fh and fh_to_dentry.
Add a function to find inodes off of a superblock, using only id.


Signed-off-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 fs/cifs/cifsfs.c    |    4 +-
 fs/cifs/cifsfs.h    |    4 +-
 fs/cifs/cifsglob.h  |    5 ++++
 fs/cifs/cifsproto.h |    2 +
 fs/cifs/dir.c       |   16 +++++--------
 fs/cifs/export.c    |   58 +++++++++++++++++++++++++++++++++++++-------------
 fs/cifs/inode.c     |   15 ++++++++++++-
 7 files changed, 74 insertions(+), 30 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 13b3999..29ff05c 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -187,12 +187,12 @@ cifs_read_super(struct super_block *sb, void *data,
 	else
 		sb->s_d_op = &cifs_dentry_ops;
 
-#ifdef CIFS_NFSD_EXPORT
+#ifdef CONFIG_CIFS_NFSD_EXPORT
 	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) {
 		cFYI(1, "export ops supported");
 		sb->s_export_op = &cifs_export_ops;
 	}
-#endif /* CIFS_NFSD_EXPORT */
+#endif /* CONFIG_CIFS_NFSD_EXPORT */
 
 	return 0;
 
diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
index 371d021..1d8185d 100644
--- a/fs/cifs/cifsfs.h
+++ b/fs/cifs/cifsfs.h
@@ -123,9 +123,9 @@ extern ssize_t	cifs_getxattr(struct dentry *, const char *, void *, size_t);
 extern ssize_t	cifs_listxattr(struct dentry *, char *, size_t);
 extern long cifs_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
 
-#ifdef CIFS_NFSD_EXPORT
+#ifdef CONFIG_CIFS_NFSD_EXPORT
 extern const struct export_operations cifs_export_ops;
-#endif /* CIFS_NFSD_EXPORT */
+#endif /* CONFIG_CIFS_NFSD_EXPORT */
 
 #define CIFS_VERSION   "1.71"
 #endif				/* _CIFSFS_H */
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index e1098c3..c7faac6 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -672,6 +672,11 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param,
 	kfree(param);
 }
 
+struct cifs_fid {
+	u64 cino;
+	u64 cpino;
+};
+
 #define   MID_FREE 0
 #define   MID_REQUEST_ALLOCATED 1
 #define   MID_REQUEST_SUBMITTED 2
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index da7a492..0d0afd2 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -432,6 +432,8 @@ extern int mdfour(unsigned char *, unsigned char *, int);
 extern int E_md4hash(const unsigned char *passwd, unsigned char *p16);
 extern void SMBencrypt(unsigned char *passwd, const unsigned char *c8,
 			unsigned char *p24);
+extern int cifs_find_inode_id(struct inode *, void *);
+extern int cifs_init_inode(struct inode *, void *);
 extern void E_P16(unsigned char *p14, unsigned char *p16);
 extern void E_P24(unsigned char *p21, const unsigned char *c8,
 			unsigned char *p24);
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index dd5f229..4e9c38f 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -491,6 +491,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
 	struct inode *newInode = NULL;
 	char *full_path = NULL;
 	struct file *filp;
+	struct dentry *spdirentry = NULL;
 
 	xid = GetXid();
 
@@ -586,7 +587,9 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
 				parent_dir_inode->i_sb, xid, NULL);
 
 	if ((rc == 0) && (newInode != NULL)) {
-		d_add(direntry, newInode);
+		spdirentry = d_splice_alias(newInode, direntry);
+		if (spdirentry)
+			return spdirentry;
 		if (posix_open) {
 			filp = lookup_instantiate_filp(nd, direntry,
 						       generic_file_open);
@@ -631,7 +634,7 @@ lookup_out:
 static int
 cifs_d_revalidate(struct dentry *direntry, struct nameidata *nd)
 {
-	if (nd->flags & LOOKUP_RCU)
+	if (nd && nd->flags & LOOKUP_RCU)
 		return -ECHILD;
 
 	if (direntry->d_inode) {
@@ -642,18 +645,11 @@ cifs_d_revalidate(struct dentry *direntry, struct nameidata *nd)
 	}
 
 	/*
-	 * This may be nfsd (or something), anyway, we can't see the
-	 * intent of this. So, since this can be for creation, drop it.
-	 */
-	if (!nd)
-		return 0;
-
-	/*
 	 * Drop the negative dentry, in order to make sure to use the
 	 * case sensitive name which is specified by user if this is
 	 * for creation.
 	 */
-	if (!(nd->flags & (LOOKUP_CONTINUE | LOOKUP_PARENT))) {
+	if (nd && !(nd->flags & (LOOKUP_CONTINUE | LOOKUP_PARENT))) {
 		if (nd->flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET))
 			return 0;
 	}
diff --git a/fs/cifs/export.c b/fs/cifs/export.c
index 55d87ac..fd8397f 100644
--- a/fs/cifs/export.c
+++ b/fs/cifs/export.c
@@ -44,24 +44,52 @@
 #include "cifsglob.h"
 #include "cifs_debug.h"
 #include "cifsfs.h"
+#include "cifsproto.h"
 
-#ifdef CIFS_NFSD_EXPORT
-static struct dentry *cifs_get_parent(struct dentry *dentry)
+#ifdef CONFIG_CIFS_NFSD_EXPORT
+static int cifs_encode_fh(struct dentry *dentry, __u32 *fh, int *lenp,
+				int connectable)
 {
-	/* BB need to add code here eventually to enable export via NFSD */
-	cFYI(1, "get parent for %p", dentry);
-	return ERR_PTR(-EACCES);
+	struct cifs_fid *cfid = (struct cifs_fid *)fh;
+	struct inode *inode = dentry->d_inode;
+
+	cfid->cino = inode->i_ino;
+
+	/*
+	 * There is not a type to send ino32_gen32_parentino32_gen32 to
+	 * accomodate two 64 bit inode numbers. So always return this type.
+	 */
+	return FILEID_INO32_GEN;
 }
 
-const struct export_operations cifs_export_ops = {
-	.get_parent = cifs_get_parent,
-/*	Following five export operations are unneeded so far and can default:
-	.get_dentry =
-	.get_name =
-	.find_exported_dentry =
-	.decode_fh =
-	.encode_fs =  */
-};
+static struct dentry *
+cifs_fh_to_dentry(struct super_block *sb, struct fid *fh,
+			int fh_len, int fh_type)
+{
+	struct cifs_fid *cfid = (struct cifs_fid *)fh;
+	struct inode *inode = NULL;
+	struct cifs_fattr fattr;
+
 
-#endif /* CIFS_NFSD_EXPORT */
+	if (fh_type != FILEID_INO32_GEN && fh_type != FILEID_INO32_GEN_PARENT) {
+		cERROR(1, "%s: Can't handle fh type: %d", __func__, fh_type);
+		return ERR_PTR(-EINVAL);
+	}
 
+	if (!cfid->cino)
+		return ERR_PTR(-ESTALE);
+
+	fattr.cf_uniqueid = cfid->cino;
+	inode = iget5_locked(sb, cfid->cino, cifs_find_inode_id,
+					cifs_init_inode, &fattr);
+	if (IS_ERR(inode))
+		return ERR_CAST(inode);
+
+	return d_obtain_alias(inode);
+}
+
+const struct export_operations cifs_export_ops = {
+	.encode_fh = cifs_encode_fh,
+	.fh_to_dentry = cifs_fh_to_dentry
+};
+#endif /* CONFIG_CIFS_NFSD_EXPORT */
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 196ef60..9c86535 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -777,6 +777,19 @@ char *cifs_build_path_to_root(struct cifs_sb_info *cifs_sb,
 	return full_path;
 }
 
+int
+cifs_find_inode_id(struct inode *inode, void *opaque)
+{
+	int rc = 0;
+	struct cifs_fattr *fattr = (struct cifs_fattr *) opaque;
+
+	/* match inode with uniqueid */
+	if (CIFS_I(inode)->uniqueid == fattr->cf_uniqueid)
+		rc = 1;
+
+	return rc;
+}
+
 static int
 cifs_find_inode(struct inode *inode, void *opaque)
 {
@@ -801,7 +814,7 @@ cifs_find_inode(struct inode *inode, void *opaque)
 	return 1;
 }
 
-static int
+int
 cifs_init_inode(struct inode *inode, void *opaque)
 {
 	struct cifs_fattr *fattr = (struct cifs_fattr *) opaque;
-- 
1.6.0.2

Re: [PATCH] cifs: Allow nfsd over cifs

[J. Bruce Fields]

On Mon, Feb 28, 2011 at 03:52:41PM -0600, shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> +static struct dentry *
> +cifs_fh_to_dentry(struct super_block *sb, struct fid *fh,
> +			int fh_len, int fh_type)
> +{
> +	struct cifs_fid *cfid = (struct cifs_fid *)fh;
> +	struct inode *inode = NULL;
> +	struct cifs_fattr fattr;
> +
>  
> -#endif /* CIFS_NFSD_EXPORT */
> +	if (fh_type != FILEID_INO32_GEN && fh_type != FILEID_INO32_GEN_PARENT) {
> +		cERROR(1, "%s: Can't handle fh type: %d", __func__, fh_type);
> +		return ERR_PTR(-EINVAL);
> +	}
>  
> +	if (!cfid->cino)
> +		return ERR_PTR(-ESTALE);
> +
> +	fattr.cf_uniqueid = cfid->cino;
> +	inode = iget5_locked(sb, cfid->cino, cifs_find_inode_id,
> +					cifs_init_inode, &fattr);
> +	if (IS_ERR(inode))
> +		return ERR_CAST(inode);
> +
> +	return d_obtain_alias(inode);

Does the cifs protocol give the client a way to look up a file by inode
number (or filehandle or equivalent?).

--b.

Re: [PATCH] cifs: Allow nfsd over cifs

[Steve French]

On Mon, Feb 28, 2011 at 4:49 PM, J. Bruce Fields <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> wrote:
> On Mon, Feb 28, 2011 at 03:52:41PM -0600, shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>> +static struct dentry *
>> +cifs_fh_to_dentry(struct super_block *sb, struct fid *fh,
>> +                     int fh_len, int fh_type)
>> +{
>> +     struct cifs_fid *cfid = (struct cifs_fid *)fh;
>> +     struct inode *inode = NULL;
>> +     struct cifs_fattr fattr;
>> +
>>
>> -#endif /* CIFS_NFSD_EXPORT */
>> +     if (fh_type != FILEID_INO32_GEN && fh_type != FILEID_INO32_GEN_PARENT) {
>> +             cERROR(1, "%s: Can't handle fh type: %d", __func__, fh_type);
>> +             return ERR_PTR(-EINVAL);
>> +     }
>>
>> +     if (!cfid->cino)
>> +             return ERR_PTR(-ESTALE);
>> +
>> +     fattr.cf_uniqueid = cfid->cino;
>> +     inode = iget5_locked(sb, cfid->cino, cifs_find_inode_id,
>> +                                     cifs_init_inode, &fattr);
>> +     if (IS_ERR(inode))
>> +             return ERR_CAST(inode);
>> +
>> +     return d_obtain_alias(inode);
>
> Does the cifs protocol give the client a way to look up a file by inode
> number (or filehandle or equivalent?).

There is a unique identifier similar to inode number returned by
various info calls,
but cifs calls use either a file handle (returned by an open) or path name to
look up metadata on a file.

Not sure whether the unique identifier can be passed in as input to an
ioctl useful here.

-- 
Thanks,

Steve

Re: [PATCH] cifs: Allow nfsd over cifs

[J. Bruce Fields]

On Mon, Feb 28, 2011 at 04:58:57PM -0600, Steve French wrote:
> On Mon, Feb 28, 2011 at 4:49 PM, J. Bruce Fields <bfields-uC3wQj2KruMpug/h7KTFAQ@public.gmane.orgg> wrote:
> > On Mon, Feb 28, 2011 at 03:52:41PM -0600, shirishpargaonkar@gmail.com wrote:
> >> +static struct dentry *
> >> +cifs_fh_to_dentry(struct super_block *sb, struct fid *fh,
> >> +                     int fh_len, int fh_type)
> >> +{
> >> +     struct cifs_fid *cfid = (struct cifs_fid *)fh;
> >> +     struct inode *inode = NULL;
> >> +     struct cifs_fattr fattr;
> >> +
> >>
> >> -#endif /* CIFS_NFSD_EXPORT */
> >> +     if (fh_type != FILEID_INO32_GEN && fh_type != FILEID_INO32_GEN_PARENT) {
> >> +             cERROR(1, "%s: Can't handle fh type: %d", __func__, fh_type);
> >> +             return ERR_PTR(-EINVAL);
> >> +     }
> >>
> >> +     if (!cfid->cino)
> >> +             return ERR_PTR(-ESTALE);
> >> +
> >> +     fattr.cf_uniqueid = cfid->cino;
> >> +     inode = iget5_locked(sb, cfid->cino, cifs_find_inode_id,
> >> +                                     cifs_init_inode, &fattr);
> >> +     if (IS_ERR(inode))
> >> +             return ERR_CAST(inode);
> >> +
> >> +     return d_obtain_alias(inode);
> >
> > Does the cifs protocol give the client a way to look up a file by inode
> > number (or filehandle or equivalent?).
> 
> There is a unique identifier similar to inode number returned by
> various info calls,
> but cifs calls use either a file handle (returned by an open) or path name to
> look up metadata on a file.
> 
> Not sure whether the unique identifier can be passed in as input to an
> ioctl useful here.

OK.  Then as things stand we're stuck returning ESTALE to the client
unless we happen to have the inode they're looking for in our cache?

--b.

Re: [PATCH] cifs: Allow nfsd over cifs

[Steve French]

On Mon, Feb 28, 2011 at 5:42 PM, J. Bruce Fields <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> wrote:
> On Mon, Feb 28, 2011 at 04:58:57PM -0600, Steve French wrote:
>> On Mon, Feb 28, 2011 at 4:49 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>> > On Mon, Feb 28, 2011 at 03:52:41PM -0600, shirishpargaonkar@gmail.com wrote:
>> >> +static struct dentry *
>> >> +cifs_fh_to_dentry(struct super_block *sb, struct fid *fh,
>> >> +                     int fh_len, int fh_type)
>> >> +{
>> >> +     struct cifs_fid *cfid = (struct cifs_fid *)fh;
>> >> +     struct inode *inode = NULL;
>> >> +     struct cifs_fattr fattr;
>> >> +
>> >>
>> >> -#endif /* CIFS_NFSD_EXPORT */
>> >> +     if (fh_type != FILEID_INO32_GEN && fh_type != FILEID_INO32_GEN_PARENT) {
>> >> +             cERROR(1, "%s: Can't handle fh type: %d", __func__, fh_type);
>> >> +             return ERR_PTR(-EINVAL);
>> >> +     }
>> >>
>> >> +     if (!cfid->cino)
>> >> +             return ERR_PTR(-ESTALE);
>> >> +
>> >> +     fattr.cf_uniqueid = cfid->cino;
>> >> +     inode = iget5_locked(sb, cfid->cino, cifs_find_inode_id,
>> >> +                                     cifs_init_inode, &fattr);
>> >> +     if (IS_ERR(inode))
>> >> +             return ERR_CAST(inode);
>> >> +
>> >> +     return d_obtain_alias(inode);
>> >
>> > Does the cifs protocol give the client a way to look up a file by inode
>> > number (or filehandle or equivalent?).
>>
>> There is a unique identifier similar to inode number returned by
>> various info calls,
>> but cifs calls use either a file handle (returned by an open) or path name to
>> look up metadata on a file.
>>
>> Not sure whether the unique identifier can be passed in as input to an
>> ioctl useful here.
>
> OK.  Then as things stand we're stuck returning ESTALE to the client
> unless we happen to have the inode they're looking for in our cache?

Yes - that seems right and consistent with what I remember other file
systems doing.
IIRC LInux nfs clients handle this already.

-- 
Thanks,

Steve

Re: [PATCH] cifs: Allow nfsd over cifs

[J. Bruce Fields]

On Mon, Feb 28, 2011 at 05:52:09PM -0600, Steve French wrote:
> On Mon, Feb 28, 2011 at 5:42 PM, J. Bruce Fields <bfields-uC3wQj2KruMpug/h7KTFAQ@public.gmane.orgg> wrote:
> > On Mon, Feb 28, 2011 at 04:58:57PM -0600, Steve French wrote:
> >> On Mon, Feb 28, 2011 at 4:49 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> >> > On Mon, Feb 28, 2011 at 03:52:41PM -0600, shirishpargaonkar@gmail.com wrote:
> >> >> +static struct dentry *
> >> >> +cifs_fh_to_dentry(struct super_block *sb, struct fid *fh,
> >> >> +                     int fh_len, int fh_type)
> >> >> +{
> >> >> +     struct cifs_fid *cfid = (struct cifs_fid *)fh;
> >> >> +     struct inode *inode = NULL;
> >> >> +     struct cifs_fattr fattr;
> >> >> +
> >> >>
> >> >> -#endif /* CIFS_NFSD_EXPORT */
> >> >> +     if (fh_type != FILEID_INO32_GEN && fh_type != FILEID_INO32_GEN_PARENT) {
> >> >> +             cERROR(1, "%s: Can't handle fh type: %d", __func__, fh_type);
> >> >> +             return ERR_PTR(-EINVAL);
> >> >> +     }
> >> >>
> >> >> +     if (!cfid->cino)
> >> >> +             return ERR_PTR(-ESTALE);
> >> >> +
> >> >> +     fattr.cf_uniqueid = cfid->cino;
> >> >> +     inode = iget5_locked(sb, cfid->cino, cifs_find_inode_id,
> >> >> +                                     cifs_init_inode, &fattr);
> >> >> +     if (IS_ERR(inode))
> >> >> +             return ERR_CAST(inode);
> >> >> +
> >> >> +     return d_obtain_alias(inode);
> >> >
> >> > Does the cifs protocol give the client a way to look up a file by inode
> >> > number (or filehandle or equivalent?).
> >>
> >> There is a unique identifier similar to inode number returned by
> >> various info calls,
> >> but cifs calls use either a file handle (returned by an open) or path name to
> >> look up metadata on a file.
> >>
> >> Not sure whether the unique identifier can be passed in as input to an
> >> ioctl useful here.
> >
> > OK.  Then as things stand we're stuck returning ESTALE to the client
> > unless we happen to have the inode they're looking for in our cache?
> 
> Yes - that seems right and consistent with what I remember other file
> systems doing.

No, other filesystems are able to look up the file on disk by inode
number (or by whatever identifier they use in the filehandle).  They
don't depend on already having the inode in core.

> IIRC LInux nfs clients handle this already.

The Linux nfs client will just pass along the ESTALE to the application.

--b.

Re: [PATCH] cifs: Allow nfsd over cifs

[Steve French]

On Mon, Feb 28, 2011 at 5:59 PM, J. Bruce Fields <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> wrote:
> On Mon, Feb 28, 2011 at 05:52:09PM -0600, Steve French wrote:
>> On Mon, Feb 28, 2011 at 5:42 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>> > On Mon, Feb 28, 2011 at 04:58:57PM -0600, Steve French wrote:
>> >> On Mon, Feb 28, 2011 at 4:49 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>> >> > On Mon, Feb 28, 2011 at 03:52:41PM -0600, shirishpargaonkar@gmail.com wrote:
>> >> >> +static struct dentry *
>> >> >> +cifs_fh_to_dentry(struct super_block *sb, struct fid *fh,
>> >> >> +                     int fh_len, int fh_type)
>> >> >> +{
>> >> >> +     struct cifs_fid *cfid = (struct cifs_fid *)fh;
>> >> >> +     struct inode *inode = NULL;
>> >> >> +     struct cifs_fattr fattr;
>> >> >> +
>> >> >>
>> >> >> -#endif /* CIFS_NFSD_EXPORT */
>> >> >> +     if (fh_type != FILEID_INO32_GEN && fh_type != FILEID_INO32_GEN_PARENT) {
>> >> >> +             cERROR(1, "%s: Can't handle fh type: %d", __func__, fh_type);
>> >> >> +             return ERR_PTR(-EINVAL);
>> >> >> +     }
>> >> >>
>> >> >> +     if (!cfid->cino)
>> >> >> +             return ERR_PTR(-ESTALE);
>> >> >> +
>> >> >> +     fattr.cf_uniqueid = cfid->cino;
>> >> >> +     inode = iget5_locked(sb, cfid->cino, cifs_find_inode_id,
>> >> >> +                                     cifs_init_inode, &fattr);
>> >> >> +     if (IS_ERR(inode))
>> >> >> +             return ERR_CAST(inode);
>> >> >> +
>> >> >> +     return d_obtain_alias(inode);
>> >> >
>> >> > Does the cifs protocol give the client a way to look up a file by inode
>> >> > number (or filehandle or equivalent?).
>> >>
>> >> There is a unique identifier similar to inode number returned by
>> >> various info calls,
>> >> but cifs calls use either a file handle (returned by an open) or path name to
>> >> look up metadata on a file.
>> >>
>> >> Not sure whether the unique identifier can be passed in as input to an
>> >> ioctl useful here.
>> >
>> > OK.  Then as things stand we're stuck returning ESTALE to the client
>> > unless we happen to have the inode they're looking for in our cache?
>>
>> Yes - that seems right and consistent with what I remember other file
>> systems doing.
>
> No, other filesystems are able to look up the file on disk by inode
> number (or by whatever identifier they use in the filehandle).  They
> don't depend on already having the inode in core.

Grepping for ESTALE looks like there are dozens of places in various
fs where ESTALE can get returned ...

-- 
Thanks,

Steve

Re: [PATCH] cifs: Allow nfsd over cifs

[J. Bruce Fields]

On Mon, Feb 28, 2011 at 08:33:08PM -0600, Steve French wrote:
> On Mon, Feb 28, 2011 at 5:59 PM, J. Bruce Fields <bfields-uC3wQj2KruMpug/h7KTFAQ@public.gmane.orgg> wrote:
> > On Mon, Feb 28, 2011 at 05:52:09PM -0600, Steve French wrote:
> >> On Mon, Feb 28, 2011 at 5:42 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> >> > OK.  Then as things stand we're stuck returning ESTALE to the client
> >> > unless we happen to have the inode they're looking for in our cache?
> >>
> >> Yes - that seems right and consistent with what I remember other file
> >> systems doing.
> >
> > No, other filesystems are able to look up the file on disk by inode
> > number (or by whatever identifier they use in the filehandle).  They
> > don't depend on already having the inode in core.
> 
> Grepping for ESTALE looks like there are dozens of places in various
> fs where ESTALE can get returned ...

Certainly true.

But they do have to be able to look up any inode, regardless of whether
it is currently in cache.

Otherwise applications on the client would see ESTALE after any server
reboot, or any time an inode was forced out of cache (for whatever
reason).

That would be quite painful.

--b.

Re: [PATCH] cifs: Allow nfsd over cifs

[Shirish Pargaonkar]

On Mon, Feb 28, 2011 at 3:52 PM,  <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
>
> Define structure cifs_fid to span 64 bits of inode ids.
> Allow nfsd over cifs.
> Add export ops encode_fh and fh_to_dentry.
> Add a function to find inodes off of a superblock, using only id.
>
>
> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  fs/cifs/cifsfs.c    |    4 +-
>  fs/cifs/cifsfs.h    |    4 +-
>  fs/cifs/cifsglob.h  |    5 ++++
>  fs/cifs/cifsproto.h |    2 +
>  fs/cifs/dir.c       |   16 +++++--------
>  fs/cifs/export.c    |   58 +++++++++++++++++++++++++++++++++++++-------------
>  fs/cifs/inode.c     |   15 ++++++++++++-
>  7 files changed, 74 insertions(+), 30 deletions(-)
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 13b3999..29ff05c 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -187,12 +187,12 @@ cifs_read_super(struct super_block *sb, void *data,
>        else
>                sb->s_d_op = &cifs_dentry_ops;
>
> -#ifdef CIFS_NFSD_EXPORT
> +#ifdef CONFIG_CIFS_NFSD_EXPORT
>        if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) {
>                cFYI(1, "export ops supported");
>                sb->s_export_op = &cifs_export_ops;
>        }
> -#endif /* CIFS_NFSD_EXPORT */
> +#endif /* CONFIG_CIFS_NFSD_EXPORT */
>
>        return 0;
>
> diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
> index 371d021..1d8185d 100644
> --- a/fs/cifs/cifsfs.h
> +++ b/fs/cifs/cifsfs.h
> @@ -123,9 +123,9 @@ extern ssize_t      cifs_getxattr(struct dentry *, const char *, void *, size_t);
>  extern ssize_t cifs_listxattr(struct dentry *, char *, size_t);
>  extern long cifs_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
>
> -#ifdef CIFS_NFSD_EXPORT
> +#ifdef CONFIG_CIFS_NFSD_EXPORT
>  extern const struct export_operations cifs_export_ops;
> -#endif /* CIFS_NFSD_EXPORT */
> +#endif /* CONFIG_CIFS_NFSD_EXPORT */
>
>  #define CIFS_VERSION   "1.71"
>  #endif                         /* _CIFSFS_H */
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index e1098c3..c7faac6 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -672,6 +672,11 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param,
>        kfree(param);
>  }
>
> +struct cifs_fid {
> +       u64 cino;
> +       u64 cpino;
> +};
> +
>  #define   MID_FREE 0
>  #define   MID_REQUEST_ALLOCATED 1
>  #define   MID_REQUEST_SUBMITTED 2
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index da7a492..0d0afd2 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -432,6 +432,8 @@ extern int mdfour(unsigned char *, unsigned char *, int);
>  extern int E_md4hash(const unsigned char *passwd, unsigned char *p16);
>  extern void SMBencrypt(unsigned char *passwd, const unsigned char *c8,
>                        unsigned char *p24);
> +extern int cifs_find_inode_id(struct inode *, void *);
> +extern int cifs_init_inode(struct inode *, void *);
>  extern void E_P16(unsigned char *p14, unsigned char *p16);
>  extern void E_P24(unsigned char *p21, const unsigned char *c8,
>                        unsigned char *p24);
> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> index dd5f229..4e9c38f 100644
> --- a/fs/cifs/dir.c
> +++ b/fs/cifs/dir.c
> @@ -491,6 +491,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
>        struct inode *newInode = NULL;
>        char *full_path = NULL;
>        struct file *filp;
> +       struct dentry *spdirentry = NULL;
>
>        xid = GetXid();
>
> @@ -586,7 +587,9 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
>                                parent_dir_inode->i_sb, xid, NULL);
>
>        if ((rc == 0) && (newInode != NULL)) {
> -               d_add(direntry, newInode);
> +               spdirentry = d_splice_alias(newInode, direntry);
> +               if (spdirentry)
> +                       return spdirentry;
>                if (posix_open) {
>                        filp = lookup_instantiate_filp(nd, direntry,
>                                                       generic_file_open);
> @@ -631,7 +634,7 @@ lookup_out:
>  static int
>  cifs_d_revalidate(struct dentry *direntry, struct nameidata *nd)
>  {
> -       if (nd->flags & LOOKUP_RCU)
> +       if (nd && nd->flags & LOOKUP_RCU)
>                return -ECHILD;
>
>        if (direntry->d_inode) {
> @@ -642,18 +645,11 @@ cifs_d_revalidate(struct dentry *direntry, struct nameidata *nd)
>        }
>
>        /*
> -        * This may be nfsd (or something), anyway, we can't see the
> -        * intent of this. So, since this can be for creation, drop it.
> -        */
> -       if (!nd)
> -               return 0;
> -
> -       /*
>         * Drop the negative dentry, in order to make sure to use the
>         * case sensitive name which is specified by user if this is
>         * for creation.
>         */
> -       if (!(nd->flags & (LOOKUP_CONTINUE | LOOKUP_PARENT))) {
> +       if (nd && !(nd->flags & (LOOKUP_CONTINUE | LOOKUP_PARENT))) {
>                if (nd->flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET))
>                        return 0;
>        }
> diff --git a/fs/cifs/export.c b/fs/cifs/export.c
> index 55d87ac..fd8397f 100644
> --- a/fs/cifs/export.c
> +++ b/fs/cifs/export.c
> @@ -44,24 +44,52 @@
>  #include "cifsglob.h"
>  #include "cifs_debug.h"
>  #include "cifsfs.h"
> +#include "cifsproto.h"
>
> -#ifdef CIFS_NFSD_EXPORT
> -static struct dentry *cifs_get_parent(struct dentry *dentry)
> +#ifdef CONFIG_CIFS_NFSD_EXPORT
> +static int cifs_encode_fh(struct dentry *dentry, __u32 *fh, int *lenp,
> +                               int connectable)
>  {
> -       /* BB need to add code here eventually to enable export via NFSD */
> -       cFYI(1, "get parent for %p", dentry);
> -       return ERR_PTR(-EACCES);
> +       struct cifs_fid *cfid = (struct cifs_fid *)fh;
> +       struct inode *inode = dentry->d_inode;
> +
> +       cfid->cino = inode->i_ino;

This should be
 cfid->cino = CIFS_I(inode)->uniqueid;
instead.
Does not address the estale issue though.

> +
> +       /*
> +        * There is not a type to send ino32_gen32_parentino32_gen32 to
> +        * accomodate two 64 bit inode numbers. So always return this type.
> +        */
> +       return FILEID_INO32_GEN;
>  }
>
> -const struct export_operations cifs_export_ops = {
> -       .get_parent = cifs_get_parent,
> -/*     Following five export operations are unneeded so far and can default:
> -       .get_dentry =
> -       .get_name =
> -       .find_exported_dentry =
> -       .decode_fh =
> -       .encode_fs =  */
> -};
> +static struct dentry *
> +cifs_fh_to_dentry(struct super_block *sb, struct fid *fh,
> +                       int fh_len, int fh_type)
> +{
> +       struct cifs_fid *cfid = (struct cifs_fid *)fh;
> +       struct inode *inode = NULL;
> +       struct cifs_fattr fattr;
> +
>
> -#endif /* CIFS_NFSD_EXPORT */
> +       if (fh_type != FILEID_INO32_GEN && fh_type != FILEID_INO32_GEN_PARENT) {
> +               cERROR(1, "%s: Can't handle fh type: %d", __func__, fh_type);
> +               return ERR_PTR(-EINVAL);
> +       }
>
> +       if (!cfid->cino)
> +               return ERR_PTR(-ESTALE);
> +
> +       fattr.cf_uniqueid = cfid->cino;
> +       inode = iget5_locked(sb, cfid->cino, cifs_find_inode_id,
> +                                       cifs_init_inode, &fattr);
> +       if (IS_ERR(inode))
> +               return ERR_CAST(inode);
> +
> +       return d_obtain_alias(inode);
> +}
> +
> +const struct export_operations cifs_export_ops = {
> +       .encode_fh = cifs_encode_fh,
> +       .fh_to_dentry = cifs_fh_to_dentry
> +};
> +#endif /* CONFIG_CIFS_NFSD_EXPORT */
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 196ef60..9c86535 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -777,6 +777,19 @@ char *cifs_build_path_to_root(struct cifs_sb_info *cifs_sb,
>        return full_path;
>  }
>
> +int
> +cifs_find_inode_id(struct inode *inode, void *opaque)
> +{
> +       int rc = 0;
> +       struct cifs_fattr *fattr = (struct cifs_fattr *) opaque;
> +
> +       /* match inode with uniqueid */
> +       if (CIFS_I(inode)->uniqueid == fattr->cf_uniqueid)
> +               rc = 1;
> +
> +       return rc;
> +}
> +
>  static int
>  cifs_find_inode(struct inode *inode, void *opaque)
>  {
> @@ -801,7 +814,7 @@ cifs_find_inode(struct inode *inode, void *opaque)
>        return 1;
>  }
>
> -static int
> +int
>  cifs_init_inode(struct inode *inode, void *opaque)
>  {
>        struct cifs_fattr *fattr = (struct cifs_fattr *) opaque;
> --
> 1.6.0.2
>
>

Re: [PATCH] cifs: Allow nfsd over cifs

[J. Bruce Fields]

On Mon, Feb 28, 2011 at 10:28:08PM -0500, J. Bruce Fields wrote:
> On Mon, Feb 28, 2011 at 08:33:08PM -0600, Steve French wrote:
> > On Mon, Feb 28, 2011 at 5:59 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > > On Mon, Feb 28, 2011 at 05:52:09PM -0600, Steve French wrote:
> > >> On Mon, Feb 28, 2011 at 5:42 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > >> > OK.  Then as things stand we're stuck returning ESTALE to the client
> > >> > unless we happen to have the inode they're looking for in our cache?
> > >>
> > >> Yes - that seems right and consistent with what I remember other file
> > >> systems doing.
> > >
> > > No, other filesystems are able to look up the file on disk by inode
> > > number (or by whatever identifier they use in the filehandle).  They
> > > don't depend on already having the inode in core.
> > 
> > Grepping for ESTALE looks like there are dozens of places in various
> > fs where ESTALE can get returned ...
> 
> Certainly true.
> 
> But they do have to be able to look up any inode, regardless of whether
> it is currently in cache.
> 
> Otherwise applications on the client would see ESTALE after any server
> reboot, or any time an inode was forced out of cache (for whatever
> reason).
> 
> That would be quite painful.

So if my understanding of the cifs behavior here is correct, then I
don't believe nfs exports of cifs will be usable.

In the long term perhaps it could be possible with changes to one or the
other of the protocols: for example, perhaps future versions of the nfs
protocol could be made less reliant on long-lived filehandles.  But that
would be a major change.

--b.

Re: [PATCH] cifs: Allow nfsd over cifs

[Shirish Pargaonkar]

On Tue, Mar 1, 2011 at 12:07 PM, J. Bruce Fields <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> wrote:
> On Mon, Feb 28, 2011 at 10:28:08PM -0500, J. Bruce Fields wrote:
>> On Mon, Feb 28, 2011 at 08:33:08PM -0600, Steve French wrote:
>> > On Mon, Feb 28, 2011 at 5:59 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>> > > On Mon, Feb 28, 2011 at 05:52:09PM -0600, Steve French wrote:
>> > >> On Mon, Feb 28, 2011 at 5:42 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>> > >> > OK.  Then as things stand we're stuck returning ESTALE to the client
>> > >> > unless we happen to have the inode they're looking for in our cache?
>> > >>
>> > >> Yes - that seems right and consistent with what I remember other file
>> > >> systems doing.
>> > >
>> > > No, other filesystems are able to look up the file on disk by inode
>> > > number (or by whatever identifier they use in the filehandle).  They
>> > > don't depend on already having the inode in core.
>> >
>> > Grepping for ESTALE looks like there are dozens of places in various
>> > fs where ESTALE can get returned ...
>>
>> Certainly true.
>>
>> But they do have to be able to look up any inode, regardless of whether
>> it is currently in cache.
>>
>> Otherwise applications on the client would see ESTALE after any server
>> reboot, or any time an inode was forced out of cache (for whatever
>> reason).
>>
>> That would be quite painful.
>
> So if my understanding of the cifs behavior here is correct, then I
> don't believe nfs exports of cifs will be usable.
>
> In the long term perhaps it could be possible with changes to one or the
> other of the protocols: for example, perhaps future versions of the nfs
> protocol could be made less reliant on long-lived filehandles.  But that
> would be a major change.
>
> --b.
>

Bruce, I am not getting a picture of how NFS server would return ESTALE
error to nfs client and then on to the app for a filehandle fragment
that happens
to be coded as uniqueid by cifs during encode_fh if inode  with that uniqueid
does not happen to exist in vfs cache on the server box.

When nfs passes down  filehandle fragment (uniqueid) to cifs in fh_to_dentry,
if the inode does not exist in cache, cifs will pick a new inode and copy this
uniqueid as inode number.  Not sure what happens next for nfs server to
sense a stale file(handle fragment).

Re: [PATCH] cifs: Allow nfsd over cifs

[J. Bruce Fields]

On Tue, Mar 01, 2011 at 12:30:12PM -0600, Shirish Pargaonkar wrote:
> On Tue, Mar 1, 2011 at 12:07 PM, J. Bruce Fields <bfields-uC3wQj2KruMpug/h7KTFAQ@public.gmane.orgg> wrote:
> > On Mon, Feb 28, 2011 at 10:28:08PM -0500, J. Bruce Fields wrote:
> >> On Mon, Feb 28, 2011 at 08:33:08PM -0600, Steve French wrote:
> >> > On Mon, Feb 28, 2011 at 5:59 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> >> > > On Mon, Feb 28, 2011 at 05:52:09PM -0600, Steve French wrote:
> >> > >> On Mon, Feb 28, 2011 at 5:42 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> >> > >> > OK.  Then as things stand we're stuck returning ESTALE to the client
> >> > >> > unless we happen to have the inode they're looking for in our cache?
> >> > >>
> >> > >> Yes - that seems right and consistent with what I remember other file
> >> > >> systems doing.
> >> > >
> >> > > No, other filesystems are able to look up the file on disk by inode
> >> > > number (or by whatever identifier they use in the filehandle).  They
> >> > > don't depend on already having the inode in core.
> >> >
> >> > Grepping for ESTALE looks like there are dozens of places in various
> >> > fs where ESTALE can get returned ...
> >>
> >> Certainly true.
> >>
> >> But they do have to be able to look up any inode, regardless of whether
> >> it is currently in cache.
> >>
> >> Otherwise applications on the client would see ESTALE after any server
> >> reboot, or any time an inode was forced out of cache (for whatever
> >> reason).
> >>
> >> That would be quite painful.
> >
> > So if my understanding of the cifs behavior here is correct, then I
> > don't believe nfs exports of cifs will be usable.
> >
> > In the long term perhaps it could be possible with changes to one or the
> > other of the protocols: for example, perhaps future versions of the nfs
> > protocol could be made less reliant on long-lived filehandles.  But that
> > would be a major change.
> >
> > --b.
> >
> 
> Bruce, I am not getting a picture of how NFS server would return ESTALE
> error to nfs client and then on to the app for a filehandle fragment
> that happens
> to be coded as uniqueid by cifs during encode_fh if inode  with that uniqueid
> does not happen to exist in vfs cache on the server box.
> 
> When nfs passes down  filehandle fragment (uniqueid) to cifs in fh_to_dentry,
> if the inode does not exist in cache, cifs will pick a new inode and copy this
> uniqueid as inode number.

Oh, OK, that's not what I'd imagined.

> Not sure what happens next for nfs server to
> sense a stale file(handle fragment).

What I'd expect to happen next:

	- d_obtain_alias will find a dentry pointing at this inode.
	  (But, note: this dentry does *not* necessarily tell us anything
	  about any name of the file.  In many cases it will be a
	  DCACHE_DISCONNECTED dentry with name "" and no parent.)

	- at some later point, nfsd will attempt to open that dentry and
	  do reads or writes.

If the cifs/smb protocols give you some way to open or lookup by
uniqueid, then you can do step 2.  I understood Steve to say that they
don't.  In that case, you're stuck.  But you're right, the failure may
be something other than returning ESTALE.

--b.

Re: [PATCH] cifs: Allow nfsd over cifs

[Jeff Layton]

On Tue, 1 Mar 2011 14:21:03 -0500
"J. Bruce Fields" <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> wrote:

> On Tue, Mar 01, 2011 at 12:30:12PM -0600, Shirish Pargaonkar wrote:
> > On Tue, Mar 1, 2011 at 12:07 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > > On Mon, Feb 28, 2011 at 10:28:08PM -0500, J. Bruce Fields wrote:
> > >> On Mon, Feb 28, 2011 at 08:33:08PM -0600, Steve French wrote:
> > >> > On Mon, Feb 28, 2011 at 5:59 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > >> > > On Mon, Feb 28, 2011 at 05:52:09PM -0600, Steve French wrote:
> > >> > >> On Mon, Feb 28, 2011 at 5:42 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > >> > >> > OK.  Then as things stand we're stuck returning ESTALE to the client
> > >> > >> > unless we happen to have the inode they're looking for in our cache?
> > >> > >>
> > >> > >> Yes - that seems right and consistent with what I remember other file
> > >> > >> systems doing.
> > >> > >
> > >> > > No, other filesystems are able to look up the file on disk by inode
> > >> > > number (or by whatever identifier they use in the filehandle).  They
> > >> > > don't depend on already having the inode in core.
> > >> >
> > >> > Grepping for ESTALE looks like there are dozens of places in various
> > >> > fs where ESTALE can get returned ...
> > >>
> > >> Certainly true.
> > >>
> > >> But they do have to be able to look up any inode, regardless of whether
> > >> it is currently in cache.
> > >>
> > >> Otherwise applications on the client would see ESTALE after any server
> > >> reboot, or any time an inode was forced out of cache (for whatever
> > >> reason).
> > >>
> > >> That would be quite painful.
> > >
> > > So if my understanding of the cifs behavior here is correct, then I
> > > don't believe nfs exports of cifs will be usable.
> > >
> > > In the long term perhaps it could be possible with changes to one or the
> > > other of the protocols: for example, perhaps future versions of the nfs
> > > protocol could be made less reliant on long-lived filehandles.  But that
> > > would be a major change.
> > >
> > > --b.
> > >
> > 
> > Bruce, I am not getting a picture of how NFS server would return ESTALE
> > error to nfs client and then on to the app for a filehandle fragment
> > that happens
> > to be coded as uniqueid by cifs during encode_fh if inode  with that uniqueid
> > does not happen to exist in vfs cache on the server box.
> > 
> > When nfs passes down  filehandle fragment (uniqueid) to cifs in fh_to_dentry,
> > if the inode does not exist in cache, cifs will pick a new inode and copy this
> > uniqueid as inode number.
> 
> Oh, OK, that's not what I'd imagined.
> 

Egads. Bruce is quite right and this is just plain not going to work.
Here's one way that this will go wrong and can never be fixed. I'm
fairly certain that there are others:

Suppose that we set up a exported cifs mount. The NFS client mounts it,
opens a file and starts writing data to the inode. So far so good --
the client did a LOOKUP operation at some point, which got translated
to a QPathInfo by cifs and a positive dentry ended up in cache.

The client then buffers up some writes. Now, suppose that the server
then reboots (or that the inode got pushed out of the cache since it
wasn't in use). The server redoes the cifs mount and reexports it. The
client then tries to flush the buffered writes. The server gets a write
request but now the inode isn't in cache and we can't match up the
filehandle to anything. Server returns -ESTALE.

At that point, you're screwed. There's virtually no way for the client
to recover. We can't guarantee that the NFS client will have a valid
path to the inode even if it did want to retry the lookup, which it
really shouldn't need to do anyway.

The fact that the CIFS protocol has no way to look up inodes by
anything like an inode number really makes this whole idea dead on
arrival.
-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] cifs: Allow nfsd over cifs

[Steve French]

I vaguely remember that the same problem can occur with nfsd on a
local file system and that nfs clients have to be able to recover from
ESTALE (e.g. lookup/delete/create/lookup would fail with ESTALE
otherwise).   Don't clients handle ESTALE by relooking up the file ala

http://lwn.net/Articles/272684/

On Tue, Mar 1, 2011 at 2:09 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Tue, 1 Mar 2011 14:21:03 -0500
> "J. Bruce Fields" <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> wrote:
>
>> On Tue, Mar 01, 2011 at 12:30:12PM -0600, Shirish Pargaonkar wrote:
>> > On Tue, Mar 1, 2011 at 12:07 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>> > > On Mon, Feb 28, 2011 at 10:28:08PM -0500, J. Bruce Fields wrote:
>> > >> On Mon, Feb 28, 2011 at 08:33:08PM -0600, Steve French wrote:
>> > >> > On Mon, Feb 28, 2011 at 5:59 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>> > >> > > On Mon, Feb 28, 2011 at 05:52:09PM -0600, Steve French wrote:
>> > >> > >> On Mon, Feb 28, 2011 at 5:42 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>> > >> > >> > OK.  Then as things stand we're stuck returning ESTALE to the client
>> > >> > >> > unless we happen to have the inode they're looking for in our cache?
>> > >> > >>
>> > >> > >> Yes - that seems right and consistent with what I remember other file
>> > >> > >> systems doing.
>> > >> > >
>> > >> > > No, other filesystems are able to look up the file on disk by inode
>> > >> > > number (or by whatever identifier they use in the filehandle).  They
>> > >> > > don't depend on already having the inode in core.
>> > >> >
>> > >> > Grepping for ESTALE looks like there are dozens of places in various
>> > >> > fs where ESTALE can get returned ...
>> > >>
>> > >> Certainly true.
>> > >>
>> > >> But they do have to be able to look up any inode, regardless of whether
>> > >> it is currently in cache.
>> > >>
>> > >> Otherwise applications on the client would see ESTALE after any server
>> > >> reboot, or any time an inode was forced out of cache (for whatever
>> > >> reason).
>> > >>
>> > >> That would be quite painful.
>> > >
>> > > So if my understanding of the cifs behavior here is correct, then I
>> > > don't believe nfs exports of cifs will be usable.
>> > >
>> > > In the long term perhaps it could be possible with changes to one or the
>> > > other of the protocols: for example, perhaps future versions of the nfs
>> > > protocol could be made less reliant on long-lived filehandles.  But that
>> > > would be a major change.
>> > >
>> > > --b.
>> > >
>> >
>> > Bruce, I am not getting a picture of how NFS server would return ESTALE
>> > error to nfs client and then on to the app for a filehandle fragment
>> > that happens
>> > to be coded as uniqueid by cifs during encode_fh if inode  with that uniqueid
>> > does not happen to exist in vfs cache on the server box.
>> >
>> > When nfs passes down  filehandle fragment (uniqueid) to cifs in fh_to_dentry,
>> > if the inode does not exist in cache, cifs will pick a new inode and copy this
>> > uniqueid as inode number.
>>
>> Oh, OK, that's not what I'd imagined.
>>
>
> Egads. Bruce is quite right and this is just plain not going to work.
> Here's one way that this will go wrong and can never be fixed. I'm
> fairly certain that there are others:
>
> Suppose that we set up a exported cifs mount. The NFS client mounts it,
> opens a file and starts writing data to the inode. So far so good --
> the client did a LOOKUP operation at some point, which got translated
> to a QPathInfo by cifs and a positive dentry ended up in cache.
>
> The client then buffers up some writes. Now, suppose that the server
> then reboots (or that the inode got pushed out of the cache since it
> wasn't in use). The server redoes the cifs mount and reexports it. The
> client then tries to flush the buffered writes. The server gets a write
> request but now the inode isn't in cache and we can't match up the
> filehandle to anything. Server returns -ESTALE.
>
> At that point, you're screwed. There's virtually no way for the client
> to recover. We can't guarantee that the NFS client will have a valid
> path to the inode even if it did want to retry the lookup, which it
> really shouldn't need to do anyway.
>
> The fact that the CIFS protocol has no way to look up inodes by
> anything like an inode number really makes this whole idea dead on
> arrival.
> --
> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>



-- 
Thanks,

Steve

Re: [PATCH] cifs: Allow nfsd over cifs

[Jeff Layton]

On Tue, 1 Mar 2011 14:11:32 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> I vaguely remember that the same problem can occur with nfsd on a
> local file system and that nfs clients have to be able to recover from
> ESTALE (e.g. lookup/delete/create/lookup would fail with ESTALE
> otherwise).   Don't clients handle ESTALE by relooking up the file ala
> 
> http://lwn.net/Articles/272684/
> 

In the case of writeback, what pathname will you use? The client just
has an inode and that inode has a filehandle. The path hasn't been
dealt with since the open().

Even if it did want to retry a lookup, it's certainly possible that the
file has been renamed on the server or by another client. If that's the
case, the NFS client will have no valid path that it can reuse anyway.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] cifs: Allow nfsd over cifs

[J. Bruce Fields]

On Tue, Mar 01, 2011 at 02:11:32PM -0600, Steve French wrote:
> I vaguely remember that the same problem can occur with nfsd on a
> local file system

Not true; as I've said, exportable local filesystems know how to look up
a file on disk by filehandle.

> and that nfs clients have to be able to recover from
> ESTALE (e.g. lookup/delete/create/lookup would fail with ESTALE
> otherwise).

It is true, however, that there are some *other* cases when a server can
return ESTALE.  In those cases (such as the one Peter Staubach discusses
in more detail at http://lwn.net/Articles/272684/) there may be some
recovery that a client might reasonably attempt.

Alas, no client can be expected to recover from ESTALE errors on an
opened file--those will just be passed on to the application.

--b.

Re: [PATCH] cifs: Allow nfsd over cifs

[Steve French]

If it is a write (at least nfsv4) we don't have this issue right?  We
will have an open, the open will have a dentry in cache so we will
have an inode num cached.

On Tue, Mar 1, 2011 at 2:31 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Tue, 1 Mar 2011 14:11:32 -0600
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> I vaguely remember that the same problem can occur with nfsd on a
>> local file system and that nfs clients have to be able to recover from
>> ESTALE (e.g. lookup/delete/create/lookup would fail with ESTALE
>> otherwise).   Don't clients handle ESTALE by relooking up the file ala
>>
>> http://lwn.net/Articles/272684/
>>
>
> In the case of writeback, what pathname will you use? The client just
> has an inode and that inode has a filehandle. The path hasn't been
> dealt with since the open().
>
> Even if it did want to retry a lookup, it's certainly possible that the
> file has been renamed on the server or by another client. If that's the
> case, the NFS client will have no valid path that it can reuse anyway.
>
> --
> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>



-- 
Thanks,

Steve

Re: [PATCH] cifs: Allow nfsd over cifs

[Steve French]

On Tue, Mar 1, 2011 at 2:34 PM, J. Bruce Fields <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> wrote:
> On Tue, Mar 01, 2011 at 02:11:32PM -0600, Steve French wrote:
>> I vaguely remember that the same problem can occur with nfsd on a
>> local file system
>
> Not true; as I've said, exportable local filesystems know how to look up
> a file on disk by filehandle.
>
>> and that nfs clients have to be able to recover from
>> ESTALE (e.g. lookup/delete/create/lookup would fail with ESTALE
>> otherwise).
>
> It is true, however, that there are some *other* cases when a server can
> return ESTALE.  In those cases (such as the one Peter Staubach discusses
> in more detail at http://lwn.net/Articles/272684/) there may be some
> recovery that a client might reasonably attempt.
>
> Alas, no client can be expected to recover from ESTALE errors on an
> opened file--those will just be passed on to the application.

If this is v4 (only) wouldn't we always have an open->dentry in cache?



-- 
Thanks,

Steve

Re: [PATCH] cifs: Allow nfsd over cifs

[J. Bruce Fields]

On Tue, Mar 01, 2011 at 02:38:27PM -0600, Steve French wrote:
> On Tue, Mar 1, 2011 at 2:34 PM, J. Bruce Fields <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> wrote:
> > On Tue, Mar 01, 2011 at 02:11:32PM -0600, Steve French wrote:
> >> I vaguely remember that the same problem can occur with nfsd on a
> >> local file system
> >
> > Not true; as I've said, exportable local filesystems know how to look up
> > a file on disk by filehandle.
> >
> >> and that nfs clients have to be able to recover from
> >> ESTALE (e.g. lookup/delete/create/lookup would fail with ESTALE
> >> otherwise).
> >
> > It is true, however, that there are some *other* cases when a server can
> > return ESTALE.  In those cases (such as the one Peter Staubach discusses
> > in more detail at http://lwn.net/Articles/272684/) there may be some
> > recovery that a client might reasonably attempt.
> >
> > Alas, no client can be expected to recover from ESTALE errors on an
> > opened file--those will just be passed on to the application.
> 
> If this is v4 (only) wouldn't we always have an open->dentry in cache?

That's correct, but only as long as the server is running.  We need
reboot recovery to work as well.

Also:

	- NFSv4 uptake is still rather small as far as I know, so the v3
	  behavior is important
	- We don't currently have any way to export cifs as v4 only.

--b.

Re: [PATCH] cifs: Allow nfsd over cifs

[Jeff Layton]

On Tue, 1 Mar 2011 15:48:07 -0500
"J. Bruce Fields" <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> wrote:

> On Tue, Mar 01, 2011 at 02:38:27PM -0600, Steve French wrote:
> > On Tue, Mar 1, 2011 at 2:34 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > > On Tue, Mar 01, 2011 at 02:11:32PM -0600, Steve French wrote:
> > >> I vaguely remember that the same problem can occur with nfsd on a
> > >> local file system
> > >
> > > Not true; as I've said, exportable local filesystems know how to look up
> > > a file on disk by filehandle.
> > >
> > >> and that nfs clients have to be able to recover from
> > >> ESTALE (e.g. lookup/delete/create/lookup would fail with ESTALE
> > >> otherwise).
> > >
> > > It is true, however, that there are some *other* cases when a server can
> > > return ESTALE.  In those cases (such as the one Peter Staubach discusses
> > > in more detail at http://lwn.net/Articles/272684/) there may be some
> > > recovery that a client might reasonably attempt.
> > >
> > > Alas, no client can be expected to recover from ESTALE errors on an
> > > opened file--those will just be passed on to the application.
> > 
> > If this is v4 (only) wouldn't we always have an open->dentry in cache?
> 
> That's correct, but only as long as the server is running.  We need
> reboot recovery to work as well.
> 
> Also:
> 
> 	- NFSv4 uptake is still rather small as far as I know, so the v3
> 	  behavior is important
> 	- We don't currently have any way to export cifs as v4 only.
> 

Steve, given that the SMB protocol has no lookup by filehandle, I don't
see how this can ever reasonably work. It's really not going to be very
useful if it's randomly going to fail if the server reboots or inodes
get pushed out of the cache.

I'd prefer to see this code dropped as I don't ever see it being
something that is shippable for distros. Do you agree?

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] cifs: Allow nfsd over cifs

[Steve French]

This is useful list.  I will look through your questions, and
reposting comments to linux-cifs.

On Wed, Mar 23, 2011 at 10:58 AM, Shirish Pargaonkar
<shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Steve,
>
> Some review comments (on smb2misc.c, will review the rest).
>
>
> smb2misc.c
>
> 1.  I think we should change check_smb2_hdr to look like check_smb_hdr.
>    ProtocolId and MessageId are compared twice.
yes. agreed.
> 2.  The mid is declared as __u16 in function check_smb2_hdr but in
>    function checkSMB2, it is declared as __u64.
Good catch.

> 3.  In function checkSMB2, we probably do not need the first else.
> 4.  In function checkSMB2, perhaps we should use CIFS_MAX_MSGSIZE instead
>    of variable CIFSMaxBufSize.
This is the similar to cifs (eventually we can change the pool size
and design for
SMB2, since optimal (and maximum possible) sizes for SMB2 requests are
far larger
than for cifs but this is reasonable for initial default for large buffers
(affects readdir responses mainly, since Jeremy had done a readpages that uses
an array of pages rather than large buffers for SMB2, since it is impractical
or impossible to allocate slab buffers large enough to hold smb2's large read
responses).   The comparison is:
                   len < (CIFSMaxBufSize + MAX_CIFS_HDR_SIZE - 4)
simply because that is the size of the objects in the pool.


> 4.  Do we need to apply le64_to_cpu to smb->MessageId while comparing
>    to mid?
No.   The MessageId is opaque (same as in cifs for the Mid, albeit for SMB2
much larger and doesn't wrap).



-- 
Thanks,

Steve