[PATCH 0/3] Fix overlayfs with NFS as lowerdir

[PATCH 0/3] Fix overlayfs with NFS as lowerdir

[Goldwyn Rodrigues]

NFS as the lower directory is not working with overlayfs because
the dentry operations have not been set correctly. However,
NFS evaluates inode and sb from the dentry, which may not be correct
when used with overlayfs. So, we store the inode in nfs_open_context
(which is translated using d_select_inode) and use it for any
inode related operations.

-- 
Goldwyn

[PATCH 0/3] Fix overlayfs with NFS as lowerdir

[Goldwyn Rodrigues]

NFS as the lower directory is not working with overlayfs because
the dentry operations have not been set correctly. However,
NFS evaluates inode and sb from the dentry, which may not be correct
when used with overlayfs. So, we store the inode in nfs_open_context
(which is translated using d_select_inode) and use it for any
inode related operations.

-- 
Goldwyn

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] ovl: Add d_select_inode to reval dentry operations

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

This was missed in earlier commit because of which access to files
in distributed filesystems such as NFS would result in ENXIO
while opening the files.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/overlayfs/super.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 8d826bd..588a4b5 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -341,6 +341,7 @@ static const struct dentry_operations ovl_dentry_operations = {
 
 static const struct dentry_operations ovl_reval_dentry_operations = {
 	.d_release = ovl_dentry_release,
+	.d_select_inode = ovl_d_select_inode,
 	.d_revalidate = ovl_dentry_revalidate,
 	.d_weak_revalidate = ovl_dentry_weak_revalidate,
 };
-- 
2.6.2

[PATCH 2/3] vfs: Add d_select_inode for overlayfs translation

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

d_select_inode() is a helper function to translate dentry to inodes
while using in conjunction with overlayfs so dentries evaluate to
true lower inodes.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 include/linux/dcache.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 7781ce11..dbaa420 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -541,6 +541,21 @@ static inline struct inode *d_inode(const struct dentry *dentry)
 }
 
 /**
+ * d_select_inode - Get the actual inode of this dentry
+ * @dentry: The dentry to query
+ * @flags: open flags passed
+ *
+ * This is the helper to select the underlying true inode associated with
+ * a dentry to cover cases of translating overlay filesystem.
+ */
+static inline struct inode *d_select_inode(struct dentry *dentry, int flags)
+{
+	if (dentry->d_flags & DCACHE_OP_SELECT_INODE)
+		return dentry->d_op->d_select_inode(dentry, flags);
+	return dentry->d_inode;
+}
+
+/**
  * d_inode_rcu - Get the actual inode of this dentry with ACCESS_ONCE()
  * @dentry: The dentry to query
  *
-- 
2.6.2

[PATCH 3/3] nfs: Store and use inode in nfs_open_context

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

NFS translates the inode from the dentry and uses sb from the dentry
parameters. However, using NFS in conjunction with overlayfs, the inodes
associated with dentries may be associated with overlayfs as opposed
to NFS. So, store inode in nfs_open_context and use d_select_inode()
to translate dentry to inode.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/nfs/dir.c           |  2 +-
 fs/nfs/inode.c         | 21 +++++++++++----------
 fs/nfs/nfs4file.c      |  2 +-
 fs/nfs/nfs4proc.c      |  2 +-
 include/linux/nfs_fs.h |  3 ++-
 5 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 9cce670..a9e0ffd 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1446,7 +1446,7 @@ static fmode_t flags_to_mode(int flags)
 
 static struct nfs_open_context *create_nfs_open_context(struct dentry *dentry, int open_flags)
 {
-	return alloc_nfs_open_context(dentry, flags_to_mode(open_flags));
+	return alloc_nfs_open_context(dentry, d_select_inode(dentry, open_flags), flags_to_mode(open_flags));
 }
 
 static int do_open(struct inode *inode, struct file *filp)
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 86faecf..64fce4b 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -728,7 +728,7 @@ static struct nfs_lock_context *__nfs_find_lock_context(struct nfs_open_context
 struct nfs_lock_context *nfs_get_lock_context(struct nfs_open_context *ctx)
 {
 	struct nfs_lock_context *res, *new = NULL;
-	struct inode *inode = d_inode(ctx->dentry);
+	struct inode *inode = ctx->inode;
 
 	spin_lock(&inode->i_lock);
 	res = __nfs_find_lock_context(ctx);
@@ -756,7 +756,7 @@ EXPORT_SYMBOL_GPL(nfs_get_lock_context);
 void nfs_put_lock_context(struct nfs_lock_context *l_ctx)
 {
 	struct nfs_open_context *ctx = l_ctx->open_context;
-	struct inode *inode = d_inode(ctx->dentry);
+	struct inode *inode = ctx->inode;
 
 	if (!atomic_dec_and_lock(&l_ctx->count, &inode->i_lock))
 		return;
@@ -785,7 +785,7 @@ void nfs_close_context(struct nfs_open_context *ctx, int is_sync)
 		return;
 	if (!is_sync)
 		return;
-	inode = d_inode(ctx->dentry);
+	inode = ctx->inode;
 	nfsi = NFS_I(inode);
 	if (inode->i_mapping->nrpages == 0)
 		return;
@@ -800,7 +800,7 @@ void nfs_close_context(struct nfs_open_context *ctx, int is_sync)
 }
 EXPORT_SYMBOL_GPL(nfs_close_context);
 
-struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, fmode_t f_mode)
+struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, struct inode *inode, fmode_t f_mode)
 {
 	struct nfs_open_context *ctx;
 	struct rpc_cred *cred = rpc_lookup_cred();
@@ -812,8 +812,9 @@ struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, fmode_t f
 		put_rpccred(cred);
 		return ERR_PTR(-ENOMEM);
 	}
-	nfs_sb_active(dentry->d_sb);
+	nfs_sb_active(inode->i_sb);
 	ctx->dentry = dget(dentry);
+	ctx->inode = inode;
 	ctx->cred = cred;
 	ctx->state = NULL;
 	ctx->mode = f_mode;
@@ -837,8 +838,8 @@ EXPORT_SYMBOL_GPL(get_nfs_open_context);
 
 static void __put_nfs_open_context(struct nfs_open_context *ctx, int is_sync)
 {
-	struct inode *inode = d_inode(ctx->dentry);
-	struct super_block *sb = ctx->dentry->d_sb;
+	struct inode *inode = ctx->inode;
+	struct super_block *sb = ctx->inode->i_sb;
 
 	if (!list_empty(&ctx->list)) {
 		if (!atomic_dec_and_lock(&ctx->lock_context.count, &inode->i_lock))
@@ -874,7 +875,7 @@ static void put_nfs_open_context_sync(struct nfs_open_context *ctx)
  */
 void nfs_inode_attach_open_context(struct nfs_open_context *ctx)
 {
-	struct inode *inode = d_inode(ctx->dentry);
+	struct inode *inode = ctx->inode;
 	struct nfs_inode *nfsi = NFS_I(inode);
 
 	spin_lock(&inode->i_lock);
@@ -917,7 +918,7 @@ void nfs_file_clear_open_context(struct file *filp)
 	struct nfs_open_context *ctx = nfs_file_open_context(filp);
 
 	if (ctx) {
-		struct inode *inode = d_inode(ctx->dentry);
+		struct inode *inode = ctx->inode;
 
 		/*
 		 * We fatal error on write before. Try to writeback
@@ -940,7 +941,7 @@ int nfs_open(struct inode *inode, struct file *filp)
 {
 	struct nfs_open_context *ctx;
 
-	ctx = alloc_nfs_open_context(filp->f_path.dentry, filp->f_mode);
+	ctx = alloc_nfs_open_context(filp->f_path.dentry, inode, filp->f_mode);
 	if (IS_ERR(ctx))
 		return PTR_ERR(ctx);
 	nfs_file_set_open_context(filp, ctx);
diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
index 57ca1c8..c7be33d 100644
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -57,7 +57,7 @@ nfs4_file_open(struct inode *inode, struct file *filp)
 	parent = dget_parent(dentry);
 	dir = d_inode(parent);
 
-	ctx = alloc_nfs_open_context(filp->f_path.dentry, filp->f_mode);
+	ctx = alloc_nfs_open_context(filp->f_path.dentry, inode, filp->f_mode);
 	err = PTR_ERR(ctx);
 	if (IS_ERR(ctx))
 		goto out;
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 1488159..57cffb9 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3707,7 +3707,7 @@ nfs4_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
 	struct nfs4_state *state;
 	int status = 0;
 
-	ctx = alloc_nfs_open_context(dentry, FMODE_READ);
+	ctx = alloc_nfs_open_context(dentry, d_select_inode(dentry, flags), FMODE_READ);
 	if (IS_ERR(ctx))
 		return PTR_ERR(ctx);
 
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 67300f8..1f18164 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -72,6 +72,7 @@ struct nfs4_state;
 struct nfs_open_context {
 	struct nfs_lock_context lock_context;
 	struct dentry *dentry;
+	struct inode *inode;
 	struct rpc_cred *cred;
 	struct nfs4_state *state;
 	fmode_t mode;
@@ -361,7 +362,7 @@ extern void nfs_setsecurity(struct inode *inode, struct nfs_fattr *fattr,
 extern struct nfs_open_context *get_nfs_open_context(struct nfs_open_context *ctx);
 extern void put_nfs_open_context(struct nfs_open_context *ctx);
 extern struct nfs_open_context *nfs_find_open_context(struct inode *inode, struct rpc_cred *cred, fmode_t mode);
-extern struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, fmode_t f_mode);
+extern struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, struct inode *inode, fmode_t f_mode);
 extern void nfs_inode_attach_open_context(struct nfs_open_context *ctx);
 extern void nfs_file_set_open_context(struct file *filp, struct nfs_open_context *ctx);
 extern void nfs_file_clear_open_context(struct file *flip);
-- 
2.6.2

[PATCH 3/3] nfs: Store and use inode in nfs_open_context

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn-IBi9RG/b67k@public.gmane.org>

NFS translates the inode from the dentry and uses sb from the dentry
parameters. However, using NFS in conjunction with overlayfs, the inodes
associated with dentries may be associated with overlayfs as opposed
to NFS. So, store inode in nfs_open_context and use d_select_inode()
to translate dentry to inode.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn-IBi9RG/b67k@public.gmane.org>
---
 fs/nfs/dir.c           |  2 +-
 fs/nfs/inode.c         | 21 +++++++++++----------
 fs/nfs/nfs4file.c      |  2 +-
 fs/nfs/nfs4proc.c      |  2 +-
 include/linux/nfs_fs.h |  3 ++-
 5 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 9cce670..a9e0ffd 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1446,7 +1446,7 @@ static fmode_t flags_to_mode(int flags)
 
 static struct nfs_open_context *create_nfs_open_context(struct dentry *dentry, int open_flags)
 {
-	return alloc_nfs_open_context(dentry, flags_to_mode(open_flags));
+	return alloc_nfs_open_context(dentry, d_select_inode(dentry, open_flags), flags_to_mode(open_flags));
 }
 
 static int do_open(struct inode *inode, struct file *filp)
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 86faecf..64fce4b 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -728,7 +728,7 @@ static struct nfs_lock_context *__nfs_find_lock_context(struct nfs_open_context
 struct nfs_lock_context *nfs_get_lock_context(struct nfs_open_context *ctx)
 {
 	struct nfs_lock_context *res, *new = NULL;
-	struct inode *inode = d_inode(ctx->dentry);
+	struct inode *inode = ctx->inode;
 
 	spin_lock(&inode->i_lock);
 	res = __nfs_find_lock_context(ctx);
@@ -756,7 +756,7 @@ EXPORT_SYMBOL_GPL(nfs_get_lock_context);
 void nfs_put_lock_context(struct nfs_lock_context *l_ctx)
 {
 	struct nfs_open_context *ctx = l_ctx->open_context;
-	struct inode *inode = d_inode(ctx->dentry);
+	struct inode *inode = ctx->inode;
 
 	if (!atomic_dec_and_lock(&l_ctx->count, &inode->i_lock))
 		return;
@@ -785,7 +785,7 @@ void nfs_close_context(struct nfs_open_context *ctx, int is_sync)
 		return;
 	if (!is_sync)
 		return;
-	inode = d_inode(ctx->dentry);
+	inode = ctx->inode;
 	nfsi = NFS_I(inode);
 	if (inode->i_mapping->nrpages == 0)
 		return;
@@ -800,7 +800,7 @@ void nfs_close_context(struct nfs_open_context *ctx, int is_sync)
 }
 EXPORT_SYMBOL_GPL(nfs_close_context);
 
-struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, fmode_t f_mode)
+struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, struct inode *inode, fmode_t f_mode)
 {
 	struct nfs_open_context *ctx;
 	struct rpc_cred *cred = rpc_lookup_cred();
@@ -812,8 +812,9 @@ struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, fmode_t f
 		put_rpccred(cred);
 		return ERR_PTR(-ENOMEM);
 	}
-	nfs_sb_active(dentry->d_sb);
+	nfs_sb_active(inode->i_sb);
 	ctx->dentry = dget(dentry);
+	ctx->inode = inode;
 	ctx->cred = cred;
 	ctx->state = NULL;
 	ctx->mode = f_mode;
@@ -837,8 +838,8 @@ EXPORT_SYMBOL_GPL(get_nfs_open_context);
 
 static void __put_nfs_open_context(struct nfs_open_context *ctx, int is_sync)
 {
-	struct inode *inode = d_inode(ctx->dentry);
-	struct super_block *sb = ctx->dentry->d_sb;
+	struct inode *inode = ctx->inode;
+	struct super_block *sb = ctx->inode->i_sb;
 
 	if (!list_empty(&ctx->list)) {
 		if (!atomic_dec_and_lock(&ctx->lock_context.count, &inode->i_lock))
@@ -874,7 +875,7 @@ static void put_nfs_open_context_sync(struct nfs_open_context *ctx)
  */
 void nfs_inode_attach_open_context(struct nfs_open_context *ctx)
 {
-	struct inode *inode = d_inode(ctx->dentry);
+	struct inode *inode = ctx->inode;
 	struct nfs_inode *nfsi = NFS_I(inode);
 
 	spin_lock(&inode->i_lock);
@@ -917,7 +918,7 @@ void nfs_file_clear_open_context(struct file *filp)
 	struct nfs_open_context *ctx = nfs_file_open_context(filp);
 
 	if (ctx) {
-		struct inode *inode = d_inode(ctx->dentry);
+		struct inode *inode = ctx->inode;
 
 		/*
 		 * We fatal error on write before. Try to writeback
@@ -940,7 +941,7 @@ int nfs_open(struct inode *inode, struct file *filp)
 {
 	struct nfs_open_context *ctx;
 
-	ctx = alloc_nfs_open_context(filp->f_path.dentry, filp->f_mode);
+	ctx = alloc_nfs_open_context(filp->f_path.dentry, inode, filp->f_mode);
 	if (IS_ERR(ctx))
 		return PTR_ERR(ctx);
 	nfs_file_set_open_context(filp, ctx);
diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
index 57ca1c8..c7be33d 100644
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -57,7 +57,7 @@ nfs4_file_open(struct inode *inode, struct file *filp)
 	parent = dget_parent(dentry);
 	dir = d_inode(parent);
 
-	ctx = alloc_nfs_open_context(filp->f_path.dentry, filp->f_mode);
+	ctx = alloc_nfs_open_context(filp->f_path.dentry, inode, filp->f_mode);
 	err = PTR_ERR(ctx);
 	if (IS_ERR(ctx))
 		goto out;
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 1488159..57cffb9 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3707,7 +3707,7 @@ nfs4_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
 	struct nfs4_state *state;
 	int status = 0;
 
-	ctx = alloc_nfs_open_context(dentry, FMODE_READ);
+	ctx = alloc_nfs_open_context(dentry, d_select_inode(dentry, flags), FMODE_READ);
 	if (IS_ERR(ctx))
 		return PTR_ERR(ctx);
 
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 67300f8..1f18164 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -72,6 +72,7 @@ struct nfs4_state;
 struct nfs_open_context {
 	struct nfs_lock_context lock_context;
 	struct dentry *dentry;
+	struct inode *inode;
 	struct rpc_cred *cred;
 	struct nfs4_state *state;
 	fmode_t mode;
@@ -361,7 +362,7 @@ extern void nfs_setsecurity(struct inode *inode, struct nfs_fattr *fattr,
 extern struct nfs_open_context *get_nfs_open_context(struct nfs_open_context *ctx);
 extern void put_nfs_open_context(struct nfs_open_context *ctx);
 extern struct nfs_open_context *nfs_find_open_context(struct inode *inode, struct rpc_cred *cred, fmode_t mode);
-extern struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, fmode_t f_mode);
+extern struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry, struct inode *inode, fmode_t f_mode);
 extern void nfs_inode_attach_open_context(struct nfs_open_context *ctx);
 extern void nfs_file_set_open_context(struct file *filp, struct nfs_open_context *ctx);
 extern void nfs_file_clear_open_context(struct file *flip);
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] nfs: Store and use inode in nfs_open_context

[Trond Myklebust]

On Tue, Mar 1, 2016 at 1:12 PM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>
> NFS translates the inode from the dentry and uses sb from the dentry
> parameters. However, using NFS in conjunction with overlayfs, the inodes
> associated with dentries may be associated with overlayfs as opposed
> to NFS. So, store inode in nfs_open_context and use d_select_inode()
> to translate dentry to inode.

I don't see how this helps. The dentry and dentry->d_sb that are
associated with the open context need to be NFS namespace objects,
otherwise all sorts of things, ranging from inode lookup to NFSv4
state recovery are going to break.

Trond

Re: [PATCH 3/3] nfs: Store and use inode in nfs_open_context

[Trond Myklebust]

On Tue, Mar 1, 2016 at 1:12 PM, Goldwyn Rodrigues <rgoldwyn-l3A5Bk7waGM@public.gmane.org> wrote:
>
> From: Goldwyn Rodrigues <rgoldwyn-IBi9RG/b67k@public.gmane.org>
>
> NFS translates the inode from the dentry and uses sb from the dentry
> parameters. However, using NFS in conjunction with overlayfs, the inodes
> associated with dentries may be associated with overlayfs as opposed
> to NFS. So, store inode in nfs_open_context and use d_select_inode()
> to translate dentry to inode.

I don't see how this helps. The dentry and dentry->d_sb that are
associated with the open context need to be NFS namespace objects,
otherwise all sorts of things, ranging from inode lookup to NFSv4
state recovery are going to break.

Trond
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] nfs: Store and use inode in nfs_open_context

[Goldwyn Rodrigues]

On 03/01/2016 02:46 PM, Trond Myklebust wrote:
> On Tue, Mar 1, 2016 at 1:12 PM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>>
>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>
>> NFS translates the inode from the dentry and uses sb from the dentry
>> parameters. However, using NFS in conjunction with overlayfs, the inodes
>> associated with dentries may be associated with overlayfs as opposed
>> to NFS. So, store inode in nfs_open_context and use d_select_inode()
>> to translate dentry to inode.
>
> I don't see how this helps. The dentry and dentry->d_sb that are
> associated with the open context need to be NFS namespace objects,
> otherwise all sorts of things, ranging from inode lookup to NFSv4
> state recovery are going to break.
>

dentry evaluations and inode lookups are done by overlayfs, with the 
help of NFS. NFS becomes a subset of overlayfs. However, you are right. 
state recovery will break with this patch.

Which makes me wonder: Shouldn't nfs_open_context (or any open context) 
be with respect to an inode as opposed to a dentry?

-- 
Goldwyn

Re: [PATCH 3/3] nfs: Store and use inode in nfs_open_context

[Trond Myklebust]

On Wed, Mar 2, 2016 at 9:27 AM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>
>
>
> On 03/01/2016 02:46 PM, Trond Myklebust wrote:
>>
>> On Tue, Mar 1, 2016 at 1:12 PM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>>>
>>>
>>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>>
>>> NFS translates the inode from the dentry and uses sb from the dentry
>>> parameters. However, using NFS in conjunction with overlayfs, the inodes
>>> associated with dentries may be associated with overlayfs as opposed
>>> to NFS. So, store inode in nfs_open_context and use d_select_inode()
>>> to translate dentry to inode.
>>
>>
>> I don't see how this helps. The dentry and dentry->d_sb that are
>> associated with the open context need to be NFS namespace objects,
>> otherwise all sorts of things, ranging from inode lookup to NFSv4
>> state recovery are going to break.
>>
>
> dentry evaluations and inode lookups are done by overlayfs, with the help of NFS. NFS becomes a subset of overlayfs. However, you are right. state recovery will break with this patch.
>
> Which makes me wonder: Shouldn't nfs_open_context (or any open context) be with respect to an inode as opposed to a dentry?

No. It is designed the way it is precisely because it needs namespace
information.

Re: [PATCH 3/3] nfs: Store and use inode in nfs_open_context

[Trond Myklebust]

On Wed, Mar 2, 2016 at 9:31 AM, Trond Myklebust
<trond.myklebust@primarydata.com> wrote:
> On Wed, Mar 2, 2016 at 9:27 AM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>>
>>
>>
>> On 03/01/2016 02:46 PM, Trond Myklebust wrote:
>>>
>>> On Tue, Mar 1, 2016 at 1:12 PM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>>>>
>>>>
>>>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>>>
>>>> NFS translates the inode from the dentry and uses sb from the dentry
>>>> parameters. However, using NFS in conjunction with overlayfs, the inodes
>>>> associated with dentries may be associated with overlayfs as opposed
>>>> to NFS. So, store inode in nfs_open_context and use d_select_inode()
>>>> to translate dentry to inode.
>>>
>>>
>>> I don't see how this helps. The dentry and dentry->d_sb that are
>>> associated with the open context need to be NFS namespace objects,
>>> otherwise all sorts of things, ranging from inode lookup to NFSv4
>>> state recovery are going to break.
>>>
>>
>> dentry evaluations and inode lookups are done by overlayfs, with the help of NFS. NFS becomes a subset of overlayfs. However, you are right. state recovery will break with this patch.
>>
>> Which makes me wonder: Shouldn't nfs_open_context (or any open context) be with respect to an inode as opposed to a dentry?
>
> No. It is designed the way it is precisely because it needs namespace
> information.

IOW: this has never been intended to be an overlayfs object. It needs
to reflect the _real_ NFS namespace for various reasons (including
recovery).

Re: [PATCH 3/3] nfs: Store and use inode in nfs_open_context

[Miklos Szeredi]

On Wed, Mar 2, 2016 at 3:38 PM, Trond Myklebust
<trond.myklebust@primarydata.com> wrote:
> On Wed, Mar 2, 2016 at 9:31 AM, Trond Myklebust
> <trond.myklebust@primarydata.com> wrote:
>> On Wed, Mar 2, 2016 at 9:27 AM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>>>
>>>
>>>
>>> On 03/01/2016 02:46 PM, Trond Myklebust wrote:
>>>>
>>>> On Tue, Mar 1, 2016 at 1:12 PM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>>>>>
>>>>>
>>>>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>>>>
>>>>> NFS translates the inode from the dentry and uses sb from the dentry
>>>>> parameters. However, using NFS in conjunction with overlayfs, the inodes
>>>>> associated with dentries may be associated with overlayfs as opposed
>>>>> to NFS. So, store inode in nfs_open_context and use d_select_inode()
>>>>> to translate dentry to inode.
>>>>
>>>>
>>>> I don't see how this helps. The dentry and dentry->d_sb that are
>>>> associated with the open context need to be NFS namespace objects,
>>>> otherwise all sorts of things, ranging from inode lookup to NFSv4
>>>> state recovery are going to break.
>>>>
>>>
>>> dentry evaluations and inode lookups are done by overlayfs, with the help of NFS. NFS becomes a subset of overlayfs. However, you are right. state recovery will break with this patch.
>>>
>>> Which makes me wonder: Shouldn't nfs_open_context (or any open context) be with respect to an inode as opposed to a dentry?
>>
>> No. It is designed the way it is precisely because it needs namespace
>> information.
>
> IOW: this has never been intended to be an overlayfs object. It needs
> to reflect the _real_ NFS namespace for various reasons (including
> recovery).


Not sure how any dentry seen by NFS became associated with overlayfs.
Through ->f_path by any chance?

Thanks,
Miklos

Re: [PATCH 3/3] nfs: Store and use inode in nfs_open_context

[Goldwyn Rodrigues]

On 03/02/2016 08:43 AM, Miklos Szeredi wrote:
<snip>

>
>
> Not sure how any dentry seen by NFS became associated with overlayfs.
> Through ->f_path by any chance?
>

Yes, NFS extracts dentry from filp->f_path.dentry in fs/nfs/inode.c 
nfs_open(). What can it use to evaluate dentry?

-- 
Goldwyn

Re: [PATCH 3/3] nfs: Store and use inode in nfs_open_context

[Miklos Szeredi]

On Wed, Mar 2, 2016 at 4:57 PM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
> On 03/02/2016 08:43 AM, Miklos Szeredi wrote:

>> Not sure how any dentry seen by NFS became associated with overlayfs.
>> Through ->f_path by any chance?
>>
>
> Yes, NFS extracts dentry from filp->f_path.dentry in fs/nfs/inode.c
> nfs_open(). What can it use to evaluate dentry?

Commit 4bacc9c9234c ("overlayfs: Make f_path always point to the
overlay and f_inode to the underlay") broke this.  Breakage only
affects regular files.  Accessing file->f_path.dentry->d_name (and
"%pD" format etc) is OK for everything.   ->d_fsdata is not OK.

I have no idea what the plan was with filesystems that use
f_path.dentry a separate open file for them.  David?  Al?

My plan was to introduce a file_dentry() helper that MUST be used by
filesystems to get the dentry from the file and that makes sure it's
the right one (check against file_inode()).   If not, then we could
call into overlayfs to return the right one, similar to
->d_select_inode(), except we want to have a dentry and we want to
have *a particular dentry* matching file_inode() (the file could have
been copied up in the mean time).

Thanks,
Miklos

Thanks,
Miklos

Re: [PATCH 3/3] nfs: Store and use inode in nfs_open_context

[Miklos Szeredi]

On Wed, Mar 2, 2016 at 4:57 PM, Goldwyn Rodrigues <rgoldwyn-l3A5Bk7waGM@public.gmane.org> wrote:
> On 03/02/2016 08:43 AM, Miklos Szeredi wrote:

>> Not sure how any dentry seen by NFS became associated with overlayfs.
>> Through ->f_path by any chance?
>>
>
> Yes, NFS extracts dentry from filp->f_path.dentry in fs/nfs/inode.c
> nfs_open(). What can it use to evaluate dentry?

Commit 4bacc9c9234c ("overlayfs: Make f_path always point to the
overlay and f_inode to the underlay") broke this.  Breakage only
affects regular files.  Accessing file->f_path.dentry->d_name (and
"%pD" format etc) is OK for everything.   ->d_fsdata is not OK.

I have no idea what the plan was with filesystems that use
f_path.dentry a separate open file for them.  David?  Al?

My plan was to introduce a file_dentry() helper that MUST be used by
filesystems to get the dentry from the file and that makes sure it's
the right one (check against file_inode()).   If not, then we could
call into overlayfs to return the right one, similar to
->d_select_inode(), except we want to have a dentry and we want to
have *a particular dentry* matching file_inode() (the file could have
been copied up in the mean time).

Thanks,
Miklos

Thanks,
Miklos
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] nfs: Store and use inode in nfs_open_context

[Miklos Szeredi]

On Thu, Mar 03, 2016 at 09:16:33AM +0100, Miklos Szeredi wrote:
> My plan was to introduce a file_dentry() helper that MUST be used by
> filesystems to get the dentry from the file and that makes sure it's
> the right one (check against file_inode()).   If not, then we could
> call into overlayfs to return the right one, similar to
> ->d_select_inode(), except we want to have a dentry and we want to
> have *a particular dentry* matching file_inode() (the file could have
> been copied up in the mean time).

Something like the following (untested, against overlayfs-next.git)

Thanks,
Miklos

---
 fs/nfs/dir.c           |    6 +++---
 fs/nfs/inode.c         |    2 +-
 fs/nfs/nfs4file.c      |    4 ++--
 fs/open.c              |   11 +++++++++++
 fs/overlayfs/super.c   |   16 ++++++++++++++++
 include/linux/dcache.h |    1 +
 include/linux/fs.h     |    2 ++
 7 files changed, 36 insertions(+), 6 deletions(-)

--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -377,7 +377,7 @@ int nfs_readdir_xdr_filler(struct page *
  again:
 	timestamp = jiffies;
 	gencount = nfs_inc_attr_generation_counter();
-	error = NFS_PROTO(inode)->readdir(file->f_path.dentry, cred, entry->cookie, pages,
+	error = NFS_PROTO(inode)->readdir(file_dentry(file), cred, entry->cookie, pages,
 					  NFS_SERVER(inode)->dtsize, desc->plus);
 	if (error < 0) {
 		/* We requested READDIRPLUS, but the server doesn't grok it */
@@ -560,7 +560,7 @@ int nfs_readdir_page_filler(nfs_readdir_
 		count++;
 
 		if (desc->plus != 0)
-			nfs_prime_dcache(desc->file->f_path.dentry, entry);
+			nfs_prime_dcache(file_dentry(desc->file), entry);
 
 		status = nfs_readdir_add_to_array(entry, page);
 		if (status != 0)
@@ -864,7 +864,7 @@ static bool nfs_dir_mapping_need_revalid
  */
 static int nfs_readdir(struct file *file, struct dir_context *ctx)
 {
-	struct dentry	*dentry = file->f_path.dentry;
+	struct dentry	*dentry = file_dentry(file);
 	struct inode	*inode = d_inode(dentry);
 	nfs_readdir_descriptor_t my_desc,
 			*desc = &my_desc;
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -940,7 +940,7 @@ int nfs_open(struct inode *inode, struct
 {
 	struct nfs_open_context *ctx;
 
-	ctx = alloc_nfs_open_context(filp->f_path.dentry, filp->f_mode);
+	ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode);
 	if (IS_ERR(ctx))
 		return PTR_ERR(ctx);
 	nfs_file_set_open_context(filp, ctx);
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -26,7 +26,7 @@ static int
 nfs4_file_open(struct inode *inode, struct file *filp)
 {
 	struct nfs_open_context *ctx;
-	struct dentry *dentry = filp->f_path.dentry;
+	struct dentry *dentry = file_dentry(filp);
 	struct dentry *parent = NULL;
 	struct inode *dir;
 	unsigned openflags = filp->f_flags;
@@ -57,7 +57,7 @@ nfs4_file_open(struct inode *inode, stru
 	parent = dget_parent(dentry);
 	dir = d_inode(parent);
 
-	ctx = alloc_nfs_open_context(filp->f_path.dentry, filp->f_mode);
+	ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode);
 	err = PTR_ERR(ctx);
 	if (IS_ERR(ctx))
 		goto out;
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1234,6 +1234,8 @@ static inline struct inode *file_inode(c
 	return f->f_inode;
 }
 
+extern struct dentry *file_dentry(const struct file *file);
+
 static inline int locks_lock_file_wait(struct file *filp, struct file_lock *fl)
 {
 	return locks_lock_inode_wait(file_inode(filp), fl);
--- a/fs/open.c
+++ b/fs/open.c
@@ -831,6 +831,17 @@ char *file_path(struct file *filp, char
 }
 EXPORT_SYMBOL(file_path);
 
+struct dentry *file_dentry(const struct file *file)
+{
+	struct dentry *dentry = file->f_path.dentry;
+
+	if (d_inode(dentry) == file_inode(file))
+		return dentry;
+	else
+		return dentry->d_op->d_select_dentry(dentry, file_inode(file));
+}
+EXPORT_SYMBOL(file_dentry);
+
 /**
  * vfs_open - open the file at the given path
  * @path: path to open
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -336,14 +336,30 @@ static int ovl_dentry_weak_revalidate(st
 	return ret;
 }
 
+static struct dentry *ovl_d_select_dentry(struct dentry *dentry,
+					 struct inode *inode)
+{
+	struct ovl_entry *oe = dentry->d_fsdata;
+	struct dentry *realentry = ovl_upperdentry_dereference(oe);
+
+	if (realentry && inode == d_inode(realentry))
+		return realentry;
+	realentry = __ovl_dentry_lower(oe);
+	if (realentry && inode == d_inode(realentry))
+		return realentry;
+	BUG();
+}
+
 static const struct dentry_operations ovl_dentry_operations = {
 	.d_release = ovl_dentry_release,
 	.d_select_inode = ovl_d_select_inode,
+	.d_select_dentry = ovl_d_select_dentry,
 };
 
 static const struct dentry_operations ovl_reval_dentry_operations = {
 	.d_release = ovl_dentry_release,
 	.d_select_inode = ovl_d_select_inode,
+	.d_select_dentry = ovl_d_select_dentry,
 	.d_revalidate = ovl_dentry_revalidate,
 	.d_weak_revalidate = ovl_dentry_weak_revalidate,
 };
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -161,6 +161,7 @@ struct dentry_operations {
 	struct vfsmount *(*d_automount)(struct path *);
 	int (*d_manage)(struct dentry *, bool);
 	struct inode *(*d_select_inode)(struct dentry *, unsigned);
+	struct dentry *(*d_select_dentry)(struct dentry *, struct inode *);
 } ____cacheline_aligned;
 
 /*

Re: [PATCH 3/3] nfs: Store and use inode in nfs_open_context

[Miklos Szeredi]

On Thu, Mar 03, 2016 at 09:16:33AM +0100, Miklos Szeredi wrote:
> My plan was to introduce a file_dentry() helper that MUST be used by
> filesystems to get the dentry from the file and that makes sure it's
> the right one (check against file_inode()).   If not, then we could
> call into overlayfs to return the right one, similar to
> ->d_select_inode(), except we want to have a dentry and we want to
> have *a particular dentry* matching file_inode() (the file could have
> been copied up in the mean time).

Something like the following (untested, against overlayfs-next.git)

Thanks,
Miklos

---
 fs/nfs/dir.c           |    6 +++---
 fs/nfs/inode.c         |    2 +-
 fs/nfs/nfs4file.c      |    4 ++--
 fs/open.c              |   11 +++++++++++
 fs/overlayfs/super.c   |   16 ++++++++++++++++
 include/linux/dcache.h |    1 +
 include/linux/fs.h     |    2 ++
 7 files changed, 36 insertions(+), 6 deletions(-)

--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -377,7 +377,7 @@ int nfs_readdir_xdr_filler(struct page *
  again:
 	timestamp = jiffies;
 	gencount = nfs_inc_attr_generation_counter();
-	error = NFS_PROTO(inode)->readdir(file->f_path.dentry, cred, entry->cookie, pages,
+	error = NFS_PROTO(inode)->readdir(file_dentry(file), cred, entry->cookie, pages,
 					  NFS_SERVER(inode)->dtsize, desc->plus);
 	if (error < 0) {
 		/* We requested READDIRPLUS, but the server doesn't grok it */
@@ -560,7 +560,7 @@ int nfs_readdir_page_filler(nfs_readdir_
 		count++;
 
 		if (desc->plus != 0)
-			nfs_prime_dcache(desc->file->f_path.dentry, entry);
+			nfs_prime_dcache(file_dentry(desc->file), entry);
 
 		status = nfs_readdir_add_to_array(entry, page);
 		if (status != 0)
@@ -864,7 +864,7 @@ static bool nfs_dir_mapping_need_revalid
  */
 static int nfs_readdir(struct file *file, struct dir_context *ctx)
 {
-	struct dentry	*dentry = file->f_path.dentry;
+	struct dentry	*dentry = file_dentry(file);
 	struct inode	*inode = d_inode(dentry);
 	nfs_readdir_descriptor_t my_desc,
 			*desc = &my_desc;
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -940,7 +940,7 @@ int nfs_open(struct inode *inode, struct
 {
 	struct nfs_open_context *ctx;
 
-	ctx = alloc_nfs_open_context(filp->f_path.dentry, filp->f_mode);
+	ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode);
 	if (IS_ERR(ctx))
 		return PTR_ERR(ctx);
 	nfs_file_set_open_context(filp, ctx);
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -26,7 +26,7 @@ static int
 nfs4_file_open(struct inode *inode, struct file *filp)
 {
 	struct nfs_open_context *ctx;
-	struct dentry *dentry = filp->f_path.dentry;
+	struct dentry *dentry = file_dentry(filp);
 	struct dentry *parent = NULL;
 	struct inode *dir;
 	unsigned openflags = filp->f_flags;
@@ -57,7 +57,7 @@ nfs4_file_open(struct inode *inode, stru
 	parent = dget_parent(dentry);
 	dir = d_inode(parent);
 
-	ctx = alloc_nfs_open_context(filp->f_path.dentry, filp->f_mode);
+	ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode);
 	err = PTR_ERR(ctx);
 	if (IS_ERR(ctx))
 		goto out;
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1234,6 +1234,8 @@ static inline struct inode *file_inode(c
 	return f->f_inode;
 }
 
+extern struct dentry *file_dentry(const struct file *file);
+
 static inline int locks_lock_file_wait(struct file *filp, struct file_lock *fl)
 {
 	return locks_lock_inode_wait(file_inode(filp), fl);
--- a/fs/open.c
+++ b/fs/open.c
@@ -831,6 +831,17 @@ char *file_path(struct file *filp, char
 }
 EXPORT_SYMBOL(file_path);
 
+struct dentry *file_dentry(const struct file *file)
+{
+	struct dentry *dentry = file->f_path.dentry;
+
+	if (d_inode(dentry) == file_inode(file))
+		return dentry;
+	else
+		return dentry->d_op->d_select_dentry(dentry, file_inode(file));
+}
+EXPORT_SYMBOL(file_dentry);
+
 /**
  * vfs_open - open the file at the given path
  * @path: path to open
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -336,14 +336,30 @@ static int ovl_dentry_weak_revalidate(st
 	return ret;
 }
 
+static struct dentry *ovl_d_select_dentry(struct dentry *dentry,
+					 struct inode *inode)
+{
+	struct ovl_entry *oe = dentry->d_fsdata;
+	struct dentry *realentry = ovl_upperdentry_dereference(oe);
+
+	if (realentry && inode == d_inode(realentry))
+		return realentry;
+	realentry = __ovl_dentry_lower(oe);
+	if (realentry && inode == d_inode(realentry))
+		return realentry;
+	BUG();
+}
+
 static const struct dentry_operations ovl_dentry_operations = {
 	.d_release = ovl_dentry_release,
 	.d_select_inode = ovl_d_select_inode,
+	.d_select_dentry = ovl_d_select_dentry,
 };
 
 static const struct dentry_operations ovl_reval_dentry_operations = {
 	.d_release = ovl_dentry_release,
 	.d_select_inode = ovl_d_select_inode,
+	.d_select_dentry = ovl_d_select_dentry,
 	.d_revalidate = ovl_dentry_revalidate,
 	.d_weak_revalidate = ovl_dentry_weak_revalidate,
 };
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -161,6 +161,7 @@ struct dentry_operations {
 	struct vfsmount *(*d_automount)(struct path *);
 	int (*d_manage)(struct dentry *, bool);
 	struct inode *(*d_select_inode)(struct dentry *, unsigned);
+	struct dentry *(*d_select_dentry)(struct dentry *, struct inode *);
 } ____cacheline_aligned;
 
 /*
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] nfs: Store and use inode in nfs_open_context

[Trond Myklebust]

On Fri, Mar 4, 2016 at 5:17 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Thu, Mar 03, 2016 at 09:16:33AM +0100, Miklos Szeredi wrote:
>> My plan was to introduce a file_dentry() helper that MUST be used by
>> filesystems to get the dentry from the file and that makes sure it's
>> the right one (check against file_inode()).   If not, then we could
>> call into overlayfs to return the right one, similar to
>> ->d_select_inode(), except we want to have a dentry and we want to
>> have *a particular dentry* matching file_inode() (the file could have
>> been copied up in the mean time).
>
> Something like the following (untested, against overlayfs-next.git)
>
> Thanks,
> Miklos
>
> ---
>  fs/nfs/dir.c           |    6 +++---
>  fs/nfs/inode.c         |    2 +-
>  fs/nfs/nfs4file.c      |    4 ++--
>  fs/open.c              |   11 +++++++++++
>  fs/overlayfs/super.c   |   16 ++++++++++++++++
>  include/linux/dcache.h |    1 +
>  include/linux/fs.h     |    2 ++
>  7 files changed, 36 insertions(+), 6 deletions(-)
>
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -377,7 +377,7 @@ int nfs_readdir_xdr_filler(struct page *
>   again:
>         timestamp = jiffies;
>         gencount = nfs_inc_attr_generation_counter();
> -       error = NFS_PROTO(inode)->readdir(file->f_path.dentry, cred, entry->cookie, pages,
> +       error = NFS_PROTO(inode)->readdir(file_dentry(file), cred, entry->cookie, pages,
>                                           NFS_SERVER(inode)->dtsize, desc->plus);
>         if (error < 0) {
>                 /* We requested READDIRPLUS, but the server doesn't grok it */
> @@ -560,7 +560,7 @@ int nfs_readdir_page_filler(nfs_readdir_
>                 count++;
>
>                 if (desc->plus != 0)
> -                       nfs_prime_dcache(desc->file->f_path.dentry, entry);
> +                       nfs_prime_dcache(file_dentry(desc->file), entry);
>
>                 status = nfs_readdir_add_to_array(entry, page);
>                 if (status != 0)
> @@ -864,7 +864,7 @@ static bool nfs_dir_mapping_need_revalid
>   */
>  static int nfs_readdir(struct file *file, struct dir_context *ctx)
>  {
> -       struct dentry   *dentry = file->f_path.dentry;
> +       struct dentry   *dentry = file_dentry(file);
>         struct inode    *inode = d_inode(dentry);
>         nfs_readdir_descriptor_t my_desc,
>                         *desc = &my_desc;
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -940,7 +940,7 @@ int nfs_open(struct inode *inode, struct
>  {
>         struct nfs_open_context *ctx;
>
> -       ctx = alloc_nfs_open_context(filp->f_path.dentry, filp->f_mode);
> +       ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode);
>         if (IS_ERR(ctx))
>                 return PTR_ERR(ctx);
>         nfs_file_set_open_context(filp, ctx);
> --- a/fs/nfs/nfs4file.c
> +++ b/fs/nfs/nfs4file.c
> @@ -26,7 +26,7 @@ static int
>  nfs4_file_open(struct inode *inode, struct file *filp)
>  {
>         struct nfs_open_context *ctx;
> -       struct dentry *dentry = filp->f_path.dentry;
> +       struct dentry *dentry = file_dentry(filp);
>         struct dentry *parent = NULL;
>         struct inode *dir;
>         unsigned openflags = filp->f_flags;
> @@ -57,7 +57,7 @@ nfs4_file_open(struct inode *inode, stru
>         parent = dget_parent(dentry);
>         dir = d_inode(parent);
>
> -       ctx = alloc_nfs_open_context(filp->f_path.dentry, filp->f_mode);
> +       ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode);
>         err = PTR_ERR(ctx);
>         if (IS_ERR(ctx))
>                 goto out;
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1234,6 +1234,8 @@ static inline struct inode *file_inode(c
>         return f->f_inode;
>  }
>
> +extern struct dentry *file_dentry(const struct file *file);
> +
>  static inline int locks_lock_file_wait(struct file *filp, struct file_lock *fl)
>  {
>         return locks_lock_inode_wait(file_inode(filp), fl);
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -831,6 +831,17 @@ char *file_path(struct file *filp, char
>  }
>  EXPORT_SYMBOL(file_path);
>
> +struct dentry *file_dentry(const struct file *file)
> +{
> +       struct dentry *dentry = file->f_path.dentry;
> +
> +       if (d_inode(dentry) == file_inode(file))
> +               return dentry;
> +       else
> +               return dentry->d_op->d_select_dentry(dentry, file_inode(file));
> +}
> +EXPORT_SYMBOL(file_dentry);
> +
>  /**
>   * vfs_open - open the file at the given path
>   * @path: path to open
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -336,14 +336,30 @@ static int ovl_dentry_weak_revalidate(st
>         return ret;
>  }
>
> +static struct dentry *ovl_d_select_dentry(struct dentry *dentry,
> +                                        struct inode *inode)
> +{
> +       struct ovl_entry *oe = dentry->d_fsdata;
> +       struct dentry *realentry = ovl_upperdentry_dereference(oe);
> +
> +       if (realentry && inode == d_inode(realentry))
> +               return realentry;
> +       realentry = __ovl_dentry_lower(oe);
> +       if (realentry && inode == d_inode(realentry))
> +               return realentry;
> +       BUG();
> +}
> +
>  static const struct dentry_operations ovl_dentry_operations = {
>         .d_release = ovl_dentry_release,
>         .d_select_inode = ovl_d_select_inode,
> +       .d_select_dentry = ovl_d_select_dentry,
>  };
>
>  static const struct dentry_operations ovl_reval_dentry_operations = {
>         .d_release = ovl_dentry_release,
>         .d_select_inode = ovl_d_select_inode,
> +       .d_select_dentry = ovl_d_select_dentry,
>         .d_revalidate = ovl_dentry_revalidate,
>         .d_weak_revalidate = ovl_dentry_weak_revalidate,
>  };
> --- a/include/linux/dcache.h
> +++ b/include/linux/dcache.h
> @@ -161,6 +161,7 @@ struct dentry_operations {
>         struct vfsmount *(*d_automount)(struct path *);
>         int (*d_manage)(struct dentry *, bool);
>         struct inode *(*d_select_inode)(struct dentry *, unsigned);
> +       struct dentry *(*d_select_dentry)(struct dentry *, struct inode *);
>  } ____cacheline_aligned;
>
>  /*

Thanks Miklos! That looks reasonable, assuming it tests out correctly.

Cheers
  Trond

Re: [PATCH 3/3] nfs: Store and use inode in nfs_open_context

[Trond Myklebust]

On Fri, Mar 4, 2016 at 5:17 AM, Miklos Szeredi <miklos-sUDqSbJrdHQHWmgEVkV9KA@public.gmane.org> wrote:
> On Thu, Mar 03, 2016 at 09:16:33AM +0100, Miklos Szeredi wrote:
>> My plan was to introduce a file_dentry() helper that MUST be used by
>> filesystems to get the dentry from the file and that makes sure it's
>> the right one (check against file_inode()).   If not, then we could
>> call into overlayfs to return the right one, similar to
>> ->d_select_inode(), except we want to have a dentry and we want to
>> have *a particular dentry* matching file_inode() (the file could have
>> been copied up in the mean time).
>
> Something like the following (untested, against overlayfs-next.git)
>
> Thanks,
> Miklos
>
> ---
>  fs/nfs/dir.c           |    6 +++---
>  fs/nfs/inode.c         |    2 +-
>  fs/nfs/nfs4file.c      |    4 ++--
>  fs/open.c              |   11 +++++++++++
>  fs/overlayfs/super.c   |   16 ++++++++++++++++
>  include/linux/dcache.h |    1 +
>  include/linux/fs.h     |    2 ++
>  7 files changed, 36 insertions(+), 6 deletions(-)
>
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -377,7 +377,7 @@ int nfs_readdir_xdr_filler(struct page *
>   again:
>         timestamp = jiffies;
>         gencount = nfs_inc_attr_generation_counter();
> -       error = NFS_PROTO(inode)->readdir(file->f_path.dentry, cred, entry->cookie, pages,
> +       error = NFS_PROTO(inode)->readdir(file_dentry(file), cred, entry->cookie, pages,
>                                           NFS_SERVER(inode)->dtsize, desc->plus);
>         if (error < 0) {
>                 /* We requested READDIRPLUS, but the server doesn't grok it */
> @@ -560,7 +560,7 @@ int nfs_readdir_page_filler(nfs_readdir_
>                 count++;
>
>                 if (desc->plus != 0)
> -                       nfs_prime_dcache(desc->file->f_path.dentry, entry);
> +                       nfs_prime_dcache(file_dentry(desc->file), entry);
>
>                 status = nfs_readdir_add_to_array(entry, page);
>                 if (status != 0)
> @@ -864,7 +864,7 @@ static bool nfs_dir_mapping_need_revalid
>   */
>  static int nfs_readdir(struct file *file, struct dir_context *ctx)
>  {
> -       struct dentry   *dentry = file->f_path.dentry;
> +       struct dentry   *dentry = file_dentry(file);
>         struct inode    *inode = d_inode(dentry);
>         nfs_readdir_descriptor_t my_desc,
>                         *desc = &my_desc;
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -940,7 +940,7 @@ int nfs_open(struct inode *inode, struct
>  {
>         struct nfs_open_context *ctx;
>
> -       ctx = alloc_nfs_open_context(filp->f_path.dentry, filp->f_mode);
> +       ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode);
>         if (IS_ERR(ctx))
>                 return PTR_ERR(ctx);
>         nfs_file_set_open_context(filp, ctx);
> --- a/fs/nfs/nfs4file.c
> +++ b/fs/nfs/nfs4file.c
> @@ -26,7 +26,7 @@ static int
>  nfs4_file_open(struct inode *inode, struct file *filp)
>  {
>         struct nfs_open_context *ctx;
> -       struct dentry *dentry = filp->f_path.dentry;
> +       struct dentry *dentry = file_dentry(filp);
>         struct dentry *parent = NULL;
>         struct inode *dir;
>         unsigned openflags = filp->f_flags;
> @@ -57,7 +57,7 @@ nfs4_file_open(struct inode *inode, stru
>         parent = dget_parent(dentry);
>         dir = d_inode(parent);
>
> -       ctx = alloc_nfs_open_context(filp->f_path.dentry, filp->f_mode);
> +       ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode);
>         err = PTR_ERR(ctx);
>         if (IS_ERR(ctx))
>                 goto out;
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1234,6 +1234,8 @@ static inline struct inode *file_inode(c
>         return f->f_inode;
>  }
>
> +extern struct dentry *file_dentry(const struct file *file);
> +
>  static inline int locks_lock_file_wait(struct file *filp, struct file_lock *fl)
>  {
>         return locks_lock_inode_wait(file_inode(filp), fl);
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -831,6 +831,17 @@ char *file_path(struct file *filp, char
>  }
>  EXPORT_SYMBOL(file_path);
>
> +struct dentry *file_dentry(const struct file *file)
> +{
> +       struct dentry *dentry = file->f_path.dentry;
> +
> +       if (d_inode(dentry) == file_inode(file))
> +               return dentry;
> +       else
> +               return dentry->d_op->d_select_dentry(dentry, file_inode(file));
> +}
> +EXPORT_SYMBOL(file_dentry);
> +
>  /**
>   * vfs_open - open the file at the given path
>   * @path: path to open
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -336,14 +336,30 @@ static int ovl_dentry_weak_revalidate(st
>         return ret;
>  }
>
> +static struct dentry *ovl_d_select_dentry(struct dentry *dentry,
> +                                        struct inode *inode)
> +{
> +       struct ovl_entry *oe = dentry->d_fsdata;
> +       struct dentry *realentry = ovl_upperdentry_dereference(oe);
> +
> +       if (realentry && inode == d_inode(realentry))
> +               return realentry;
> +       realentry = __ovl_dentry_lower(oe);
> +       if (realentry && inode == d_inode(realentry))
> +               return realentry;
> +       BUG();
> +}
> +
>  static const struct dentry_operations ovl_dentry_operations = {
>         .d_release = ovl_dentry_release,
>         .d_select_inode = ovl_d_select_inode,
> +       .d_select_dentry = ovl_d_select_dentry,
>  };
>
>  static const struct dentry_operations ovl_reval_dentry_operations = {
>         .d_release = ovl_dentry_release,
>         .d_select_inode = ovl_d_select_inode,
> +       .d_select_dentry = ovl_d_select_dentry,
>         .d_revalidate = ovl_dentry_revalidate,
>         .d_weak_revalidate = ovl_dentry_weak_revalidate,
>  };
> --- a/include/linux/dcache.h
> +++ b/include/linux/dcache.h
> @@ -161,6 +161,7 @@ struct dentry_operations {
>         struct vfsmount *(*d_automount)(struct path *);
>         int (*d_manage)(struct dentry *, bool);
>         struct inode *(*d_select_inode)(struct dentry *, unsigned);
> +       struct dentry *(*d_select_dentry)(struct dentry *, struct inode *);
>  } ____cacheline_aligned;
>
>  /*

Thanks Miklos! That looks reasonable, assuming it tests out correctly.

Cheers
  Trond
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] nfs: Store and use inode in nfs_open_context

[Goldwyn Rodrigues]

On 03/04/2016 07:41 AM, Trond Myklebust wrote:
> On Fri, Mar 4, 2016 at 5:17 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Thu, Mar 03, 2016 at 09:16:33AM +0100, Miklos Szeredi wrote:
>>> My plan was to introduce a file_dentry() helper that MUST be used by
>>> filesystems to get the dentry from the file and that makes sure it's
>>> the right one (check against file_inode()).   If not, then we could
>>> call into overlayfs to return the right one, similar to
>>> ->d_select_inode(), except we want to have a dentry and we want to
>>> have *a particular dentry* matching file_inode() (the file could have
>>> been copied up in the mean time).
>>
>> Something like the following (untested, against overlayfs-next.git)
>>
>> Thanks,
>> Miklos
>>
>> ---
>>   fs/nfs/dir.c           |    6 +++---
>>   fs/nfs/inode.c         |    2 +-
>>   fs/nfs/nfs4file.c      |    4 ++--
>>   fs/open.c              |   11 +++++++++++
>>   fs/overlayfs/super.c   |   16 ++++++++++++++++
>>   include/linux/dcache.h |    1 +
>>   include/linux/fs.h     |    2 ++
>>   7 files changed, 36 insertions(+), 6 deletions(-)
>>
>> --- a/fs/nfs/dir.c
>> +++ b/fs/nfs/dir.c
>> @@ -377,7 +377,7 @@ int nfs_readdir_xdr_filler(struct page *
>>    again:
>>          timestamp = jiffies;
>>          gencount = nfs_inc_attr_generation_counter();
>> -       error = NFS_PROTO(inode)->readdir(file->f_path.dentry, cred, entry->cookie, pages,
>> +       error = NFS_PROTO(inode)->readdir(file_dentry(file), cred, entry->cookie, pages,
>>                                            NFS_SERVER(inode)->dtsize, desc->plus);
>>          if (error < 0) {
>>                  /* We requested READDIRPLUS, but the server doesn't grok it */
>> @@ -560,7 +560,7 @@ int nfs_readdir_page_filler(nfs_readdir_
>>                  count++;
>>
>>                  if (desc->plus != 0)
>> -                       nfs_prime_dcache(desc->file->f_path.dentry, entry);
>> +                       nfs_prime_dcache(file_dentry(desc->file), entry);
>>
>>                  status = nfs_readdir_add_to_array(entry, page);
>>                  if (status != 0)
>> @@ -864,7 +864,7 @@ static bool nfs_dir_mapping_need_revalid
>>    */
>>   static int nfs_readdir(struct file *file, struct dir_context *ctx)
>>   {
>> -       struct dentry   *dentry = file->f_path.dentry;
>> +       struct dentry   *dentry = file_dentry(file);
>>          struct inode    *inode = d_inode(dentry);
>>          nfs_readdir_descriptor_t my_desc,
>>                          *desc = &my_desc;
>> --- a/fs/nfs/inode.c
>> +++ b/fs/nfs/inode.c
>> @@ -940,7 +940,7 @@ int nfs_open(struct inode *inode, struct
>>   {
>>          struct nfs_open_context *ctx;
>>
>> -       ctx = alloc_nfs_open_context(filp->f_path.dentry, filp->f_mode);
>> +       ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode);
>>          if (IS_ERR(ctx))
>>                  return PTR_ERR(ctx);
>>          nfs_file_set_open_context(filp, ctx);
>> --- a/fs/nfs/nfs4file.c
>> +++ b/fs/nfs/nfs4file.c
>> @@ -26,7 +26,7 @@ static int
>>   nfs4_file_open(struct inode *inode, struct file *filp)
>>   {
>>          struct nfs_open_context *ctx;
>> -       struct dentry *dentry = filp->f_path.dentry;
>> +       struct dentry *dentry = file_dentry(filp);
>>          struct dentry *parent = NULL;
>>          struct inode *dir;
>>          unsigned openflags = filp->f_flags;
>> @@ -57,7 +57,7 @@ nfs4_file_open(struct inode *inode, stru
>>          parent = dget_parent(dentry);
>>          dir = d_inode(parent);
>>
>> -       ctx = alloc_nfs_open_context(filp->f_path.dentry, filp->f_mode);
>> +       ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode);
>>          err = PTR_ERR(ctx);
>>          if (IS_ERR(ctx))
>>                  goto out;
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -1234,6 +1234,8 @@ static inline struct inode *file_inode(c
>>          return f->f_inode;
>>   }
>>
>> +extern struct dentry *file_dentry(const struct file *file);
>> +
>>   static inline int locks_lock_file_wait(struct file *filp, struct file_lock *fl)
>>   {
>>          return locks_lock_inode_wait(file_inode(filp), fl);
>> --- a/fs/open.c
>> +++ b/fs/open.c
>> @@ -831,6 +831,17 @@ char *file_path(struct file *filp, char
>>   }
>>   EXPORT_SYMBOL(file_path);
>>
>> +struct dentry *file_dentry(const struct file *file)
>> +{
>> +       struct dentry *dentry = file->f_path.dentry;
>> +
>> +       if (d_inode(dentry) == file_inode(file))
>> +               return dentry;
>> +       else
>> +               return dentry->d_op->d_select_dentry(dentry, file_inode(file));
>> +}
>> +EXPORT_SYMBOL(file_dentry);
>> +
>>   /**
>>    * vfs_open - open the file at the given path
>>    * @path: path to open
>> --- a/fs/overlayfs/super.c
>> +++ b/fs/overlayfs/super.c
>> @@ -336,14 +336,30 @@ static int ovl_dentry_weak_revalidate(st
>>          return ret;
>>   }
>>
>> +static struct dentry *ovl_d_select_dentry(struct dentry *dentry,
>> +                                        struct inode *inode)
>> +{
>> +       struct ovl_entry *oe = dentry->d_fsdata;
>> +       struct dentry *realentry = ovl_upperdentry_dereference(oe);
>> +
>> +       if (realentry && inode == d_inode(realentry))
>> +               return realentry;
>> +       realentry = __ovl_dentry_lower(oe);
>> +       if (realentry && inode == d_inode(realentry))
>> +               return realentry;
>> +       BUG();
>> +}
>> +
>>   static const struct dentry_operations ovl_dentry_operations = {
>>          .d_release = ovl_dentry_release,
>>          .d_select_inode = ovl_d_select_inode,
>> +       .d_select_dentry = ovl_d_select_dentry,
>>   };
>>
>>   static const struct dentry_operations ovl_reval_dentry_operations = {
>>          .d_release = ovl_dentry_release,
>>          .d_select_inode = ovl_d_select_inode,
>> +       .d_select_dentry = ovl_d_select_dentry,
>>          .d_revalidate = ovl_dentry_revalidate,
>>          .d_weak_revalidate = ovl_dentry_weak_revalidate,
>>   };
>> --- a/include/linux/dcache.h
>> +++ b/include/linux/dcache.h
>> @@ -161,6 +161,7 @@ struct dentry_operations {
>>          struct vfsmount *(*d_automount)(struct path *);
>>          int (*d_manage)(struct dentry *, bool);
>>          struct inode *(*d_select_inode)(struct dentry *, unsigned);
>> +       struct dentry *(*d_select_dentry)(struct dentry *, struct inode *);
>>   } ____cacheline_aligned;
>>
>>   /*
>
> Thanks Miklos! That looks reasonable, assuming it tests out correctly.
>

Tested and it looks good. Thanks Miklos.

Tested-by: Goldwyn Rodrigues <rgoldwyn@suse.com>


-- 
Goldwyn