[PATCH 0/2] rpc_pipe: clean up how dentry operations get set in rpc_pipefs

[PATCH 0/2] rpc_pipe: clean up how dentry operations get set in rpc_pipefs

[Jeff Layton]

Recently, I proposed a patch on the linux-nfs list to silence a WARNING:

    [PATCH] rpc_pipefs: only set rpc_dentry_ops if d_op isn't already set

When reviewing that, Bruce (properly) pointed out that that patch sucked
since it just hacked around the warning and did nothing to improve the
code. This patchset is an attempt to correct that by making rpc_pipefs
use a consistent set of dentry_operations, and ensuring that they're set
on every dentry at d_alloc time.

Since Trond has already merged the earlier patch I sent into his
nfs-for-next branch, this is based on top of that.

Jeff Layton (2):
  rpc_pipe: export simple_dentry_operations and have rpc_pipefs use it
  rpc_pipe: set dentry operations at d_alloc time

 fs/libfs.c            |  9 +++++----
 include/linux/fs.h    |  1 +
 net/sunrpc/rpc_pipe.c | 24 +++++++++++++++---------
 3 files changed, 21 insertions(+), 13 deletions(-)

-- 
1.8.1.4

--
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 0/2] rpc_pipe: clean up how dentry operations get set in rpc_pipefs

[Jeff Layton]

Recently, I proposed a patch on the linux-nfs list to silence a WARNING:

    [PATCH] rpc_pipefs: only set rpc_dentry_ops if d_op isn't already set

When reviewing that, Bruce (properly) pointed out that that patch sucked
since it just hacked around the warning and did nothing to improve the
code. This patchset is an attempt to correct that by making rpc_pipefs
use a consistent set of dentry_operations, and ensuring that they're set
on every dentry at d_alloc time.

Since Trond has already merged the earlier patch I sent into his
nfs-for-next branch, this is based on top of that.

Jeff Layton (2):
  rpc_pipe: export simple_dentry_operations and have rpc_pipefs use it
  rpc_pipe: set dentry operations at d_alloc time

 fs/libfs.c            |  9 +++++----
 include/linux/fs.h    |  1 +
 net/sunrpc/rpc_pipe.c | 24 +++++++++++++++---------
 3 files changed, 21 insertions(+), 13 deletions(-)

-- 
1.8.1.4

[PATCH 1/2] rpc_pipe: export simple_dentry_operations and have rpc_pipefs use it

[Jeff Layton]

...instead of replicating a copy of them.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/libfs.c            |  9 +++++----
 include/linux/fs.h    |  1 +
 net/sunrpc/rpc_pipe.c | 11 +----------
 3 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index 916da8c..69793f7 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -49,16 +49,16 @@ static int simple_delete_dentry(const struct dentry *dentry)
 	return 1;
 }
 
+const struct dentry_operations simple_dentry_operations = {
+	.d_delete = simple_delete_dentry,
+};
+
 /*
  * Lookup the data. This is trivial - if the dentry didn't already
  * exist, we know it is negative.  Set d_op to delete negative dentries.
  */
 struct dentry *simple_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
 {
-	static const struct dentry_operations simple_dentry_operations = {
-		.d_delete = simple_delete_dentry,
-	};
-
 	if (dentry->d_name.len > NAME_MAX)
 		return ERR_PTR(-ENAMETOOLONG);
 	d_set_d_op(dentry, &simple_dentry_operations);
@@ -987,6 +987,7 @@ EXPORT_SYMBOL(simple_write_begin);
 EXPORT_SYMBOL(simple_write_end);
 EXPORT_SYMBOL(simple_dir_inode_operations);
 EXPORT_SYMBOL(simple_dir_operations);
+EXPORT_SYMBOL(simple_dentry_operations);
 EXPORT_SYMBOL(simple_empty);
 EXPORT_SYMBOL(simple_fill_super);
 EXPORT_SYMBOL(simple_getattr);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 65c2be2..1d21364 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2546,6 +2546,7 @@ extern int simple_write_end(struct file *file, struct address_space *mapping,
 extern struct dentry *simple_lookup(struct inode *, struct dentry *, unsigned int flags);
 extern ssize_t generic_read_dir(struct file *, char __user *, size_t, loff_t *);
 extern const struct file_operations simple_dir_operations;
+extern const struct dentry_operations simple_dentry_operations;
 extern const struct inode_operations simple_dir_inode_operations;
 struct tree_descr { char *name; const struct file_operations *ops; int mode; };
 struct dentry *d_alloc_name(struct dentry *, const char *);
diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index a816b3a..99cd7db 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -471,15 +471,6 @@ struct rpc_filelist {
 	umode_t mode;
 };
 
-static int rpc_delete_dentry(const struct dentry *dentry)
-{
-	return 1;
-}
-
-static const struct dentry_operations rpc_dentry_operations = {
-	.d_delete = rpc_delete_dentry,
-};
-
 static struct inode *
 rpc_get_inode(struct super_block *sb, umode_t mode)
 {
@@ -668,7 +659,7 @@ static struct dentry *__rpc_lookup_create_exclusive(struct dentry *parent,
 	}
 	if (dentry->d_inode == NULL) {
 		if (!dentry->d_op)
-			d_set_d_op(dentry, &rpc_dentry_operations);
+			d_set_d_op(dentry, &simple_dentry_operations);
 		return dentry;
 	}
 	dput(dentry);
-- 
1.8.1.4

[PATCH 2/2] rpc_pipe: set dentry operations at d_alloc time

[Jeff Layton]

Currently the way these get set is a little convoluted. If the dentry is
allocated via lookup from userland, then it gets set by simple_lookup.
If it gets allocated when the kernel is populating the directory, then
it gets set via __rpc_lookup_create_exclusive, which has to check
whether they might already be set. Between both of these, this ensures
that all dentries have their d_op pointer set.

Instead of doing that, just have them set at d_alloc time by pointing
sb->s_d_op at them. With that change, we no longer want the lookup op
to set them, so we must move to using our own lookup routine.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 net/sunrpc/rpc_pipe.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index 99cd7db..7b471a2 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -471,6 +471,23 @@ struct rpc_filelist {
 	umode_t mode;
 };
 
+/*
+ * Lookup the data. This is trivial - if the dentry didn't already
+ * exist, we know it is negative.
+ */
+static struct dentry *
+rpc_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
+{
+	if (dentry->d_name.len > NAME_MAX)
+		return ERR_PTR(-ENAMETOOLONG);
+	d_add(dentry, NULL);
+	return NULL;
+}
+
+const struct inode_operations rpc_dir_inode_operations = {
+	.lookup		= rpc_lookup,
+};
+
 static struct inode *
 rpc_get_inode(struct super_block *sb, umode_t mode)
 {
@@ -483,7 +500,7 @@ rpc_get_inode(struct super_block *sb, umode_t mode)
 	switch (mode & S_IFMT) {
 	case S_IFDIR:
 		inode->i_fop = &simple_dir_operations;
-		inode->i_op = &simple_dir_inode_operations;
+		inode->i_op = &rpc_dir_inode_operations;
 		inc_nlink(inode);
 	default:
 		break;
@@ -657,11 +674,8 @@ static struct dentry *__rpc_lookup_create_exclusive(struct dentry *parent,
 		if (!dentry)
 			return ERR_PTR(-ENOMEM);
 	}
-	if (dentry->d_inode == NULL) {
-		if (!dentry->d_op)
-			d_set_d_op(dentry, &simple_dentry_operations);
+	if (dentry->d_inode == NULL)
 		return dentry;
-	}
 	dput(dentry);
 	return ERR_PTR(-EEXIST);
 }
@@ -1108,6 +1122,7 @@ rpc_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_blocksize_bits = PAGE_CACHE_SHIFT;
 	sb->s_magic = RPCAUTH_GSSMAGIC;
 	sb->s_op = &s_ops;
+	sb->s_d_op = &simple_dentry_operations;
 	sb->s_time_gran = 1;
 
 	inode = rpc_get_inode(sb, S_IFDIR | S_IRUGO | S_IXUGO);
-- 
1.8.1.4

--
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 2/2] rpc_pipe: set dentry operations at d_alloc time

[Jeff Layton]

Currently the way these get set is a little convoluted. If the dentry is
allocated via lookup from userland, then it gets set by simple_lookup.
If it gets allocated when the kernel is populating the directory, then
it gets set via __rpc_lookup_create_exclusive, which has to check
whether they might already be set. Between both of these, this ensures
that all dentries have their d_op pointer set.

Instead of doing that, just have them set at d_alloc time by pointing
sb->s_d_op at them. With that change, we no longer want the lookup op
to set them, so we must move to using our own lookup routine.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 net/sunrpc/rpc_pipe.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index 99cd7db..7b471a2 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -471,6 +471,23 @@ struct rpc_filelist {
 	umode_t mode;
 };
 
+/*
+ * Lookup the data. This is trivial - if the dentry didn't already
+ * exist, we know it is negative.
+ */
+static struct dentry *
+rpc_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
+{
+	if (dentry->d_name.len > NAME_MAX)
+		return ERR_PTR(-ENAMETOOLONG);
+	d_add(dentry, NULL);
+	return NULL;
+}
+
+const struct inode_operations rpc_dir_inode_operations = {
+	.lookup		= rpc_lookup,
+};
+
 static struct inode *
 rpc_get_inode(struct super_block *sb, umode_t mode)
 {
@@ -483,7 +500,7 @@ rpc_get_inode(struct super_block *sb, umode_t mode)
 	switch (mode & S_IFMT) {
 	case S_IFDIR:
 		inode->i_fop = &simple_dir_operations;
-		inode->i_op = &simple_dir_inode_operations;
+		inode->i_op = &rpc_dir_inode_operations;
 		inc_nlink(inode);
 	default:
 		break;
@@ -657,11 +674,8 @@ static struct dentry *__rpc_lookup_create_exclusive(struct dentry *parent,
 		if (!dentry)
 			return ERR_PTR(-ENOMEM);
 	}
-	if (dentry->d_inode == NULL) {
-		if (!dentry->d_op)
-			d_set_d_op(dentry, &simple_dentry_operations);
+	if (dentry->d_inode == NULL)
 		return dentry;
-	}
 	dput(dentry);
 	return ERR_PTR(-EEXIST);
 }
@@ -1108,6 +1122,7 @@ rpc_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_blocksize_bits = PAGE_CACHE_SHIFT;
 	sb->s_magic = RPCAUTH_GSSMAGIC;
 	sb->s_op = &s_ops;
+	sb->s_d_op = &simple_dentry_operations;
 	sb->s_time_gran = 1;
 
 	inode = rpc_get_inode(sb, S_IFDIR | S_IRUGO | S_IXUGO);
-- 
1.8.1.4

Re: [PATCH 2/2] rpc_pipe: set dentry operations at d_alloc time

[Jeff Layton]

On Mon, 24 Jun 2013 21:05:40 -0400
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> Currently the way these get set is a little convoluted. If the dentry is
> allocated via lookup from userland, then it gets set by simple_lookup.
> If it gets allocated when the kernel is populating the directory, then
> it gets set via __rpc_lookup_create_exclusive, which has to check
> whether they might already be set. Between both of these, this ensures
> that all dentries have their d_op pointer set.
> 
> Instead of doing that, just have them set at d_alloc time by pointing
> sb->s_d_op at them. With that change, we no longer want the lookup op
> to set them, so we must move to using our own lookup routine.
> 
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  net/sunrpc/rpc_pipe.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
> index 99cd7db..7b471a2 100644
> --- a/net/sunrpc/rpc_pipe.c
> +++ b/net/sunrpc/rpc_pipe.c
> @@ -471,6 +471,23 @@ struct rpc_filelist {
>  	umode_t mode;
>  };
>  
> +/*
> + * Lookup the data. This is trivial - if the dentry didn't already
> + * exist, we know it is negative.
> + */
> +static struct dentry *
> +rpc_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
> +{
> +	if (dentry->d_name.len > NAME_MAX)
> +		return ERR_PTR(-ENAMETOOLONG);
> +	d_add(dentry, NULL);
> +	return NULL;
> +}
> +
> +const struct inode_operations rpc_dir_inode_operations = {
> +	.lookup		= rpc_lookup,
> +};
> +

Nit: the above should probably be static. Let me know if you want me to respin...

>  static struct inode *
>  rpc_get_inode(struct super_block *sb, umode_t mode)
>  {
> @@ -483,7 +500,7 @@ rpc_get_inode(struct super_block *sb, umode_t mode)
>  	switch (mode & S_IFMT) {
>  	case S_IFDIR:
>  		inode->i_fop = &simple_dir_operations;
> -		inode->i_op = &simple_dir_inode_operations;
> +		inode->i_op = &rpc_dir_inode_operations;
>  		inc_nlink(inode);
>  	default:
>  		break;
> @@ -657,11 +674,8 @@ static struct dentry *__rpc_lookup_create_exclusive(struct dentry *parent,
>  		if (!dentry)
>  			return ERR_PTR(-ENOMEM);
>  	}
> -	if (dentry->d_inode == NULL) {
> -		if (!dentry->d_op)
> -			d_set_d_op(dentry, &simple_dentry_operations);
> +	if (dentry->d_inode == NULL)
>  		return dentry;
> -	}
>  	dput(dentry);
>  	return ERR_PTR(-EEXIST);
>  }
> @@ -1108,6 +1122,7 @@ rpc_fill_super(struct super_block *sb, void *data, int silent)
>  	sb->s_blocksize_bits = PAGE_CACHE_SHIFT;
>  	sb->s_magic = RPCAUTH_GSSMAGIC;
>  	sb->s_op = &s_ops;
> +	sb->s_d_op = &simple_dentry_operations;
>  	sb->s_time_gran = 1;
>  
>  	inode = rpc_get_inode(sb, S_IFDIR | S_IRUGO | S_IXUGO);


-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
--
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 2/2] rpc_pipe: set dentry operations at d_alloc time

[Jeff Layton]

On Mon, 24 Jun 2013 21:05:40 -0400
Jeff Layton <jlayton@redhat.com> wrote:

> Currently the way these get set is a little convoluted. If the dentry is
> allocated via lookup from userland, then it gets set by simple_lookup.
> If it gets allocated when the kernel is populating the directory, then
> it gets set via __rpc_lookup_create_exclusive, which has to check
> whether they might already be set. Between both of these, this ensures
> that all dentries have their d_op pointer set.
> 
> Instead of doing that, just have them set at d_alloc time by pointing
> sb->s_d_op at them. With that change, we no longer want the lookup op
> to set them, so we must move to using our own lookup routine.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  net/sunrpc/rpc_pipe.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
> index 99cd7db..7b471a2 100644
> --- a/net/sunrpc/rpc_pipe.c
> +++ b/net/sunrpc/rpc_pipe.c
> @@ -471,6 +471,23 @@ struct rpc_filelist {
>  	umode_t mode;
>  };
>  
> +/*
> + * Lookup the data. This is trivial - if the dentry didn't already
> + * exist, we know it is negative.
> + */
> +static struct dentry *
> +rpc_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
> +{
> +	if (dentry->d_name.len > NAME_MAX)
> +		return ERR_PTR(-ENAMETOOLONG);
> +	d_add(dentry, NULL);
> +	return NULL;
> +}
> +
> +const struct inode_operations rpc_dir_inode_operations = {
> +	.lookup		= rpc_lookup,
> +};
> +

Nit: the above should probably be static. Let me know if you want me to respin...

>  static struct inode *
>  rpc_get_inode(struct super_block *sb, umode_t mode)
>  {
> @@ -483,7 +500,7 @@ rpc_get_inode(struct super_block *sb, umode_t mode)
>  	switch (mode & S_IFMT) {
>  	case S_IFDIR:
>  		inode->i_fop = &simple_dir_operations;
> -		inode->i_op = &simple_dir_inode_operations;
> +		inode->i_op = &rpc_dir_inode_operations;
>  		inc_nlink(inode);
>  	default:
>  		break;
> @@ -657,11 +674,8 @@ static struct dentry *__rpc_lookup_create_exclusive(struct dentry *parent,
>  		if (!dentry)
>  			return ERR_PTR(-ENOMEM);
>  	}
> -	if (dentry->d_inode == NULL) {
> -		if (!dentry->d_op)
> -			d_set_d_op(dentry, &simple_dentry_operations);
> +	if (dentry->d_inode == NULL)
>  		return dentry;
> -	}
>  	dput(dentry);
>  	return ERR_PTR(-EEXIST);
>  }
> @@ -1108,6 +1122,7 @@ rpc_fill_super(struct super_block *sb, void *data, int silent)
>  	sb->s_blocksize_bits = PAGE_CACHE_SHIFT;
>  	sb->s_magic = RPCAUTH_GSSMAGIC;
>  	sb->s_op = &s_ops;
> +	sb->s_d_op = &simple_dentry_operations;
>  	sb->s_time_gran = 1;
>  
>  	inode = rpc_get_inode(sb, S_IFDIR | S_IRUGO | S_IXUGO);


-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 1/2] rpc_pipe: export simple_dentry_operations and have rpc_pipefs use it

[Myklebust, Trond]

On Mon, 2013-06-24 at 21:05 -0400, Jeff Layton wrote:
> ...instead of replicating a copy of them.
> 
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/libfs.c            |  9 +++++----
>  include/linux/fs.h    |  1 +
>  net/sunrpc/rpc_pipe.c | 11 +----------
>  3 files changed, 7 insertions(+), 14 deletions(-)
> 

Can you please get an ACK from Al on this patch.

Cheers
  Trond

> diff --git a/fs/libfs.c b/fs/libfs.c
> index 916da8c..69793f7 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -49,16 +49,16 @@ static int simple_delete_dentry(const struct dentry *dentry)
>  	return 1;
>  }
>  
> +const struct dentry_operations simple_dentry_operations = {
> +	.d_delete = simple_delete_dentry,
> +};
> +
>  /*
>   * Lookup the data. This is trivial - if the dentry didn't already
>   * exist, we know it is negative.  Set d_op to delete negative dentries.
>   */
>  struct dentry *simple_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
>  {
> -	static const struct dentry_operations simple_dentry_operations = {
> -		.d_delete = simple_delete_dentry,
> -	};
> -
>  	if (dentry->d_name.len > NAME_MAX)
>  		return ERR_PTR(-ENAMETOOLONG);
>  	d_set_d_op(dentry, &simple_dentry_operations);
> @@ -987,6 +987,7 @@ EXPORT_SYMBOL(simple_write_begin);
>  EXPORT_SYMBOL(simple_write_end);
>  EXPORT_SYMBOL(simple_dir_inode_operations);
>  EXPORT_SYMBOL(simple_dir_operations);
> +EXPORT_SYMBOL(simple_dentry_operations);
>  EXPORT_SYMBOL(simple_empty);
>  EXPORT_SYMBOL(simple_fill_super);
>  EXPORT_SYMBOL(simple_getattr);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 65c2be2..1d21364 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2546,6 +2546,7 @@ extern int simple_write_end(struct file *file, struct address_space *mapping,
>  extern struct dentry *simple_lookup(struct inode *, struct dentry *, unsigned int flags);
>  extern ssize_t generic_read_dir(struct file *, char __user *, size_t, loff_t *);
>  extern const struct file_operations simple_dir_operations;
> +extern const struct dentry_operations simple_dentry_operations;
>  extern const struct inode_operations simple_dir_inode_operations;
>  struct tree_descr { char *name; const struct file_operations *ops; int mode; };
>  struct dentry *d_alloc_name(struct dentry *, const char *);
> diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
> index a816b3a..99cd7db 100644
> --- a/net/sunrpc/rpc_pipe.c
> +++ b/net/sunrpc/rpc_pipe.c
> @@ -471,15 +471,6 @@ struct rpc_filelist {
>  	umode_t mode;
>  };
>  
> -static int rpc_delete_dentry(const struct dentry *dentry)
> -{
> -	return 1;
> -}
> -
> -static const struct dentry_operations rpc_dentry_operations = {
> -	.d_delete = rpc_delete_dentry,
> -};
> -
>  static struct inode *
>  rpc_get_inode(struct super_block *sb, umode_t mode)
>  {
> @@ -668,7 +659,7 @@ static struct dentry *__rpc_lookup_create_exclusive(struct dentry *parent,
>  	}
>  	if (dentry->d_inode == NULL) {
>  		if (!dentry->d_op)
> -			d_set_d_op(dentry, &rpc_dentry_operations);
> +			d_set_d_op(dentry, &simple_dentry_operations);
>  		return dentry;
>  	}
>  	dput(dentry);


-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org
www.netapp.com
--
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 1/2] rpc_pipe: export simple_dentry_operations and have rpc_pipefs use it

[Myklebust, Trond]

On Mon, 2013-06-24 at 21:05 -0400, Jeff Layton wrote:
> ...instead of replicating a copy of them.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/libfs.c            |  9 +++++----
>  include/linux/fs.h    |  1 +
>  net/sunrpc/rpc_pipe.c | 11 +----------
>  3 files changed, 7 insertions(+), 14 deletions(-)
> 

Can you please get an ACK from Al on this patch.

Cheers
  Trond

> diff --git a/fs/libfs.c b/fs/libfs.c
> index 916da8c..69793f7 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -49,16 +49,16 @@ static int simple_delete_dentry(const struct dentry *dentry)
>  	return 1;
>  }
>  
> +const struct dentry_operations simple_dentry_operations = {
> +	.d_delete = simple_delete_dentry,
> +};
> +
>  /*
>   * Lookup the data. This is trivial - if the dentry didn't already
>   * exist, we know it is negative.  Set d_op to delete negative dentries.
>   */
>  struct dentry *simple_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
>  {
> -	static const struct dentry_operations simple_dentry_operations = {
> -		.d_delete = simple_delete_dentry,
> -	};
> -
>  	if (dentry->d_name.len > NAME_MAX)
>  		return ERR_PTR(-ENAMETOOLONG);
>  	d_set_d_op(dentry, &simple_dentry_operations);
> @@ -987,6 +987,7 @@ EXPORT_SYMBOL(simple_write_begin);
>  EXPORT_SYMBOL(simple_write_end);
>  EXPORT_SYMBOL(simple_dir_inode_operations);
>  EXPORT_SYMBOL(simple_dir_operations);
> +EXPORT_SYMBOL(simple_dentry_operations);
>  EXPORT_SYMBOL(simple_empty);
>  EXPORT_SYMBOL(simple_fill_super);
>  EXPORT_SYMBOL(simple_getattr);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 65c2be2..1d21364 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2546,6 +2546,7 @@ extern int simple_write_end(struct file *file, struct address_space *mapping,
>  extern struct dentry *simple_lookup(struct inode *, struct dentry *, unsigned int flags);
>  extern ssize_t generic_read_dir(struct file *, char __user *, size_t, loff_t *);
>  extern const struct file_operations simple_dir_operations;
> +extern const struct dentry_operations simple_dentry_operations;
>  extern const struct inode_operations simple_dir_inode_operations;
>  struct tree_descr { char *name; const struct file_operations *ops; int mode; };
>  struct dentry *d_alloc_name(struct dentry *, const char *);
> diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
> index a816b3a..99cd7db 100644
> --- a/net/sunrpc/rpc_pipe.c
> +++ b/net/sunrpc/rpc_pipe.c
> @@ -471,15 +471,6 @@ struct rpc_filelist {
>  	umode_t mode;
>  };
>  
> -static int rpc_delete_dentry(const struct dentry *dentry)
> -{
> -	return 1;
> -}
> -
> -static const struct dentry_operations rpc_dentry_operations = {
> -	.d_delete = rpc_delete_dentry,
> -};
> -
>  static struct inode *
>  rpc_get_inode(struct super_block *sb, umode_t mode)
>  {
> @@ -668,7 +659,7 @@ static struct dentry *__rpc_lookup_create_exclusive(struct dentry *parent,
>  	}
>  	if (dentry->d_inode == NULL) {
>  		if (!dentry->d_op)
> -			d_set_d_op(dentry, &rpc_dentry_operations);
> +			d_set_d_op(dentry, &simple_dentry_operations);
>  		return dentry;
>  	}
>  	dput(dentry);


-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH 1/2] rpc_pipe: export simple_dentry_operations and have rpc_pipefs use it

[Jeff Layton]

On Tue, 25 Jun 2013 15:36:54 +0000
"Myklebust, Trond" <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org> wrote:

> On Mon, 2013-06-24 at 21:05 -0400, Jeff Layton wrote:
> > ...instead of replicating a copy of them.
> > 
> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  fs/libfs.c            |  9 +++++----
> >  include/linux/fs.h    |  1 +
> >  net/sunrpc/rpc_pipe.c | 11 +----------
> >  3 files changed, 7 insertions(+), 14 deletions(-)
> > 
> 
> Can you please get an ACK from Al on this patch.
> 
> Cheers
>   Trond
> 

Al, could you weigh in on this? The only real change to generic VFS
code is that I'm exporting simple_dentry_operations here.

Thanks,
Jeff

> > diff --git a/fs/libfs.c b/fs/libfs.c
> > index 916da8c..69793f7 100644
> > --- a/fs/libfs.c
> > +++ b/fs/libfs.c
> > @@ -49,16 +49,16 @@ static int simple_delete_dentry(const struct dentry *dentry)
> >  	return 1;
> >  }
> >  
> > +const struct dentry_operations simple_dentry_operations = {
> > +	.d_delete = simple_delete_dentry,
> > +};
> > +
> >  /*
> >   * Lookup the data. This is trivial - if the dentry didn't already
> >   * exist, we know it is negative.  Set d_op to delete negative dentries.
> >   */
> >  struct dentry *simple_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
> >  {
> > -	static const struct dentry_operations simple_dentry_operations = {
> > -		.d_delete = simple_delete_dentry,
> > -	};
> > -
> >  	if (dentry->d_name.len > NAME_MAX)
> >  		return ERR_PTR(-ENAMETOOLONG);
> >  	d_set_d_op(dentry, &simple_dentry_operations);
> > @@ -987,6 +987,7 @@ EXPORT_SYMBOL(simple_write_begin);
> >  EXPORT_SYMBOL(simple_write_end);
> >  EXPORT_SYMBOL(simple_dir_inode_operations);
> >  EXPORT_SYMBOL(simple_dir_operations);
> > +EXPORT_SYMBOL(simple_dentry_operations);
> >  EXPORT_SYMBOL(simple_empty);
> >  EXPORT_SYMBOL(simple_fill_super);
> >  EXPORT_SYMBOL(simple_getattr);
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 65c2be2..1d21364 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2546,6 +2546,7 @@ extern int simple_write_end(struct file *file, struct address_space *mapping,
> >  extern struct dentry *simple_lookup(struct inode *, struct dentry *, unsigned int flags);
> >  extern ssize_t generic_read_dir(struct file *, char __user *, size_t, loff_t *);
> >  extern const struct file_operations simple_dir_operations;
> > +extern const struct dentry_operations simple_dentry_operations;
> >  extern const struct inode_operations simple_dir_inode_operations;
> >  struct tree_descr { char *name; const struct file_operations *ops; int mode; };
> >  struct dentry *d_alloc_name(struct dentry *, const char *);
> > diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
> > index a816b3a..99cd7db 100644
> > --- a/net/sunrpc/rpc_pipe.c
> > +++ b/net/sunrpc/rpc_pipe.c
> > @@ -471,15 +471,6 @@ struct rpc_filelist {
> >  	umode_t mode;
> >  };
> >  
> > -static int rpc_delete_dentry(const struct dentry *dentry)
> > -{
> > -	return 1;
> > -}
> > -
> > -static const struct dentry_operations rpc_dentry_operations = {
> > -	.d_delete = rpc_delete_dentry,
> > -};
> > -
> >  static struct inode *
> >  rpc_get_inode(struct super_block *sb, umode_t mode)
> >  {
> > @@ -668,7 +659,7 @@ static struct dentry *__rpc_lookup_create_exclusive(struct dentry *parent,
> >  	}
> >  	if (dentry->d_inode == NULL) {
> >  		if (!dentry->d_op)
> > -			d_set_d_op(dentry, &rpc_dentry_operations);
> > +			d_set_d_op(dentry, &simple_dentry_operations);
> >  		return dentry;
> >  	}
> >  	dput(dentry);
> 
> 


-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
--
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 1/2] rpc_pipe: export simple_dentry_operations and have rpc_pipefs use it

[Jeff Layton]

On Tue, 25 Jun 2013 15:36:54 +0000
"Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:

> On Mon, 2013-06-24 at 21:05 -0400, Jeff Layton wrote:
> > ...instead of replicating a copy of them.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  fs/libfs.c            |  9 +++++----
> >  include/linux/fs.h    |  1 +
> >  net/sunrpc/rpc_pipe.c | 11 +----------
> >  3 files changed, 7 insertions(+), 14 deletions(-)
> > 
> 
> Can you please get an ACK from Al on this patch.
> 
> Cheers
>   Trond
> 

Al, could you weigh in on this? The only real change to generic VFS
code is that I'm exporting simple_dentry_operations here.

Thanks,
Jeff

> > diff --git a/fs/libfs.c b/fs/libfs.c
> > index 916da8c..69793f7 100644
> > --- a/fs/libfs.c
> > +++ b/fs/libfs.c
> > @@ -49,16 +49,16 @@ static int simple_delete_dentry(const struct dentry *dentry)
> >  	return 1;
> >  }
> >  
> > +const struct dentry_operations simple_dentry_operations = {
> > +	.d_delete = simple_delete_dentry,
> > +};
> > +
> >  /*
> >   * Lookup the data. This is trivial - if the dentry didn't already
> >   * exist, we know it is negative.  Set d_op to delete negative dentries.
> >   */
> >  struct dentry *simple_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
> >  {
> > -	static const struct dentry_operations simple_dentry_operations = {
> > -		.d_delete = simple_delete_dentry,
> > -	};
> > -
> >  	if (dentry->d_name.len > NAME_MAX)
> >  		return ERR_PTR(-ENAMETOOLONG);
> >  	d_set_d_op(dentry, &simple_dentry_operations);
> > @@ -987,6 +987,7 @@ EXPORT_SYMBOL(simple_write_begin);
> >  EXPORT_SYMBOL(simple_write_end);
> >  EXPORT_SYMBOL(simple_dir_inode_operations);
> >  EXPORT_SYMBOL(simple_dir_operations);
> > +EXPORT_SYMBOL(simple_dentry_operations);
> >  EXPORT_SYMBOL(simple_empty);
> >  EXPORT_SYMBOL(simple_fill_super);
> >  EXPORT_SYMBOL(simple_getattr);
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 65c2be2..1d21364 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2546,6 +2546,7 @@ extern int simple_write_end(struct file *file, struct address_space *mapping,
> >  extern struct dentry *simple_lookup(struct inode *, struct dentry *, unsigned int flags);
> >  extern ssize_t generic_read_dir(struct file *, char __user *, size_t, loff_t *);
> >  extern const struct file_operations simple_dir_operations;
> > +extern const struct dentry_operations simple_dentry_operations;
> >  extern const struct inode_operations simple_dir_inode_operations;
> >  struct tree_descr { char *name; const struct file_operations *ops; int mode; };
> >  struct dentry *d_alloc_name(struct dentry *, const char *);
> > diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
> > index a816b3a..99cd7db 100644
> > --- a/net/sunrpc/rpc_pipe.c
> > +++ b/net/sunrpc/rpc_pipe.c
> > @@ -471,15 +471,6 @@ struct rpc_filelist {
> >  	umode_t mode;
> >  };
> >  
> > -static int rpc_delete_dentry(const struct dentry *dentry)
> > -{
> > -	return 1;
> > -}
> > -
> > -static const struct dentry_operations rpc_dentry_operations = {
> > -	.d_delete = rpc_delete_dentry,
> > -};
> > -
> >  static struct inode *
> >  rpc_get_inode(struct super_block *sb, umode_t mode)
> >  {
> > @@ -668,7 +659,7 @@ static struct dentry *__rpc_lookup_create_exclusive(struct dentry *parent,
> >  	}
> >  	if (dentry->d_inode == NULL) {
> >  		if (!dentry->d_op)
> > -			d_set_d_op(dentry, &rpc_dentry_operations);
> > +			d_set_d_op(dentry, &simple_dentry_operations);
> >  		return dentry;
> >  	}
> >  	dput(dentry);
> 
> 


-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 1/2] rpc_pipe: export simple_dentry_operations and have rpc_pipefs use it

[Jeff Layton]

On Mon, 2013-07-01 at 12:22 -0400, Jeff Layton wrote:
> On Tue, 25 Jun 2013 15:36:54 +0000
> "Myklebust, Trond" <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org> wrote:
> 
> > On Mon, 2013-06-24 at 21:05 -0400, Jeff Layton wrote:
> > > ...instead of replicating a copy of them.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > ---
> > >  fs/libfs.c            |  9 +++++----
> > >  include/linux/fs.h    |  1 +
> > >  net/sunrpc/rpc_pipe.c | 11 +----------
> > >  3 files changed, 7 insertions(+), 14 deletions(-)
> > > 
> > 
> > Can you please get an ACK from Al on this patch.
> > 
> > Cheers
> >   Trond
> > 
> 
> Al, could you weigh in on this? The only real change to generic VFS
> code is that I'm exporting simple_dentry_operations here.
> 

Trond, I've been unable to get in touch with Al to get him to ACK this.
I'm guessing he's busy getting patches together for the merge window. 

I propose we do this instead:

I'll respin the patch such that sb->s_d_op is set to
rpc_dentry_operations, and we'll just keep those for now. Then, I can
propose a patch later to export simple_dentry_operations and convert
rpc_pipefs to use that afterward (probably for 3.12).

I'll post the respin after I've done a bit of testing...

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

--
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 1/2] rpc_pipe: export simple_dentry_operations and have rpc_pipefs use it

[Jeff Layton]

On Mon, 2013-07-01 at 12:22 -0400, Jeff Layton wrote:
> On Tue, 25 Jun 2013 15:36:54 +0000
> "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:
> 
> > On Mon, 2013-06-24 at 21:05 -0400, Jeff Layton wrote:
> > > ...instead of replicating a copy of them.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > ---
> > >  fs/libfs.c            |  9 +++++----
> > >  include/linux/fs.h    |  1 +
> > >  net/sunrpc/rpc_pipe.c | 11 +----------
> > >  3 files changed, 7 insertions(+), 14 deletions(-)
> > > 
> > 
> > Can you please get an ACK from Al on this patch.
> > 
> > Cheers
> >   Trond
> > 
> 
> Al, could you weigh in on this? The only real change to generic VFS
> code is that I'm exporting simple_dentry_operations here.
> 

Trond, I've been unable to get in touch with Al to get him to ACK this.
I'm guessing he's busy getting patches together for the merge window. 

I propose we do this instead:

I'll respin the patch such that sb->s_d_op is set to
rpc_dentry_operations, and we'll just keep those for now. Then, I can
propose a patch later to export simple_dentry_operations and convert
rpc_pipefs to use that afterward (probably for 3.12).

I'll post the respin after I've done a bit of testing...

-- 
Jeff Layton <jlayton@redhat.com>

[PATCH v2] rpc_pipe: set dentry operations at d_alloc time

[Jeff Layton]

Currently the way these get set is a little convoluted. If the dentry is
allocated via lookup from userland, then it gets set by simple_lookup.
If it gets allocated when the kernel is populating the directory, then
it gets set via __rpc_lookup_create_exclusive, which has to check
whether they might already be set. Between both of these, this ensures
that all dentries have their d_op pointer set.

Instead of doing that, just have them set at d_alloc time by pointing
sb->s_d_op at them. With that change, we no longer want the lookup op
to set them, so we must move to using our own lookup routine.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 net/sunrpc/rpc_pipe.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index a816b3a..ca5ad70d 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -480,6 +480,23 @@ static const struct dentry_operations rpc_dentry_operations = {
 	.d_delete = rpc_delete_dentry,
 };
 
+/*
+ * Lookup the data. This is trivial - if the dentry didn't already
+ * exist, we know it is negative.
+ */
+static struct dentry *
+rpc_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
+{
+	if (dentry->d_name.len > NAME_MAX)
+		return ERR_PTR(-ENAMETOOLONG);
+	d_add(dentry, NULL);
+	return NULL;
+}
+
+const struct inode_operations rpc_dir_inode_operations = {
+	.lookup		= rpc_lookup,
+};
+
 static struct inode *
 rpc_get_inode(struct super_block *sb, umode_t mode)
 {
@@ -492,7 +509,7 @@ rpc_get_inode(struct super_block *sb, umode_t mode)
 	switch (mode & S_IFMT) {
 	case S_IFDIR:
 		inode->i_fop = &simple_dir_operations;
-		inode->i_op = &simple_dir_inode_operations;
+		inode->i_op = &rpc_dir_inode_operations;
 		inc_nlink(inode);
 	default:
 		break;
@@ -666,11 +683,8 @@ static struct dentry *__rpc_lookup_create_exclusive(struct dentry *parent,
 		if (!dentry)
 			return ERR_PTR(-ENOMEM);
 	}
-	if (dentry->d_inode == NULL) {
-		if (!dentry->d_op)
-			d_set_d_op(dentry, &rpc_dentry_operations);
+	if (dentry->d_inode == NULL)
 		return dentry;
-	}
 	dput(dentry);
 	return ERR_PTR(-EEXIST);
 }
@@ -1117,6 +1131,7 @@ rpc_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_blocksize_bits = PAGE_CACHE_SHIFT;
 	sb->s_magic = RPCAUTH_GSSMAGIC;
 	sb->s_op = &s_ops;
+	sb->s_d_op = &rpc_dentry_operations;
 	sb->s_time_gran = 1;
 
 	inode = rpc_get_inode(sb, S_IFDIR | S_IRUGO | S_IXUGO);
-- 
1.8.1.4

--
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 v2] rpc_pipe: set dentry operations at d_alloc time

[Jeff Layton]

Currently the way these get set is a little convoluted. If the dentry is
allocated via lookup from userland, then it gets set by simple_lookup.
If it gets allocated when the kernel is populating the directory, then
it gets set via __rpc_lookup_create_exclusive, which has to check
whether they might already be set. Between both of these, this ensures
that all dentries have their d_op pointer set.

Instead of doing that, just have them set at d_alloc time by pointing
sb->s_d_op at them. With that change, we no longer want the lookup op
to set them, so we must move to using our own lookup routine.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 net/sunrpc/rpc_pipe.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index a816b3a..ca5ad70d 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -480,6 +480,23 @@ static const struct dentry_operations rpc_dentry_operations = {
 	.d_delete = rpc_delete_dentry,
 };
 
+/*
+ * Lookup the data. This is trivial - if the dentry didn't already
+ * exist, we know it is negative.
+ */
+static struct dentry *
+rpc_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
+{
+	if (dentry->d_name.len > NAME_MAX)
+		return ERR_PTR(-ENAMETOOLONG);
+	d_add(dentry, NULL);
+	return NULL;
+}
+
+const struct inode_operations rpc_dir_inode_operations = {
+	.lookup		= rpc_lookup,
+};
+
 static struct inode *
 rpc_get_inode(struct super_block *sb, umode_t mode)
 {
@@ -492,7 +509,7 @@ rpc_get_inode(struct super_block *sb, umode_t mode)
 	switch (mode & S_IFMT) {
 	case S_IFDIR:
 		inode->i_fop = &simple_dir_operations;
-		inode->i_op = &simple_dir_inode_operations;
+		inode->i_op = &rpc_dir_inode_operations;
 		inc_nlink(inode);
 	default:
 		break;
@@ -666,11 +683,8 @@ static struct dentry *__rpc_lookup_create_exclusive(struct dentry *parent,
 		if (!dentry)
 			return ERR_PTR(-ENOMEM);
 	}
-	if (dentry->d_inode == NULL) {
-		if (!dentry->d_op)
-			d_set_d_op(dentry, &rpc_dentry_operations);
+	if (dentry->d_inode == NULL)
 		return dentry;
-	}
 	dput(dentry);
 	return ERR_PTR(-EEXIST);
 }
@@ -1117,6 +1131,7 @@ rpc_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_blocksize_bits = PAGE_CACHE_SHIFT;
 	sb->s_magic = RPCAUTH_GSSMAGIC;
 	sb->s_op = &s_ops;
+	sb->s_d_op = &rpc_dentry_operations;
 	sb->s_time_gran = 1;
 
 	inode = rpc_get_inode(sb, S_IFDIR | S_IRUGO | S_IXUGO);
-- 
1.8.1.4

Re: [PATCH v2] rpc_pipe: set dentry operations at d_alloc time

[Al Viro]

On Tue, Jul 02, 2013 at 01:00:52PM -0400, Jeff Layton wrote:
> Currently the way these get set is a little convoluted. If the dentry is
> allocated via lookup from userland, then it gets set by simple_lookup.
> If it gets allocated when the kernel is populating the directory, then
> it gets set via __rpc_lookup_create_exclusive, which has to check
> whether they might already be set. Between both of these, this ensures
> that all dentries have their d_op pointer set.
> 
> Instead of doing that, just have them set at d_alloc time by pointing
> sb->s_d_op at them. With that change, we no longer want the lookup op
> to set them, so we must move to using our own lookup routine.

There's a better solution - just make simple_lookup() skip d_set_d_op()
if superblock already has ->s_d_op (and thus d_alloc() has already
set the damn thing).  Voila - we can just set ->s_d_op and leave
inode_operations as is.
--
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 v2] rpc_pipe: set dentry operations at d_alloc time

[Al Viro]

On Tue, Jul 02, 2013 at 01:00:52PM -0400, Jeff Layton wrote:
> Currently the way these get set is a little convoluted. If the dentry is
> allocated via lookup from userland, then it gets set by simple_lookup.
> If it gets allocated when the kernel is populating the directory, then
> it gets set via __rpc_lookup_create_exclusive, which has to check
> whether they might already be set. Between both of these, this ensures
> that all dentries have their d_op pointer set.
> 
> Instead of doing that, just have them set at d_alloc time by pointing
> sb->s_d_op at them. With that change, we no longer want the lookup op
> to set them, so we must move to using our own lookup routine.

There's a better solution - just make simple_lookup() skip d_set_d_op()
if superblock already has ->s_d_op (and thus d_alloc() has already
set the damn thing).  Voila - we can just set ->s_d_op and leave
inode_operations as is.

Re: [PATCH v2] rpc_pipe: set dentry operations at d_alloc time

[Jeff Layton]

On Sun, 14 Jul 2013 15:00:11 +0100
Al Viro <viro-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org> wrote:

> On Tue, Jul 02, 2013 at 01:00:52PM -0400, Jeff Layton wrote:
> > Currently the way these get set is a little convoluted. If the dentry is
> > allocated via lookup from userland, then it gets set by simple_lookup.
> > If it gets allocated when the kernel is populating the directory, then
> > it gets set via __rpc_lookup_create_exclusive, which has to check
> > whether they might already be set. Between both of these, this ensures
> > that all dentries have their d_op pointer set.
> > 
> > Instead of doing that, just have them set at d_alloc time by pointing
> > sb->s_d_op at them. With that change, we no longer want the lookup op
> > to set them, so we must move to using our own lookup routine.
> 
> There's a better solution - just make simple_lookup() skip d_set_d_op()
> if superblock already has ->s_d_op (and thus d_alloc() has already
> set the damn thing).  Voila - we can just set ->s_d_op and leave
> inode_operations as is.

Yeah, that is a better solution and the code now in mainline looks
reasonable. Trond already merged this patch however, so I'll spin up
another patch on top of mainline to convert this back to
simple_dir_inode_operations.

Thanks,
-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
--
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 v2] rpc_pipe: set dentry operations at d_alloc time

[Jeff Layton]

On Sun, 14 Jul 2013 15:00:11 +0100
Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Tue, Jul 02, 2013 at 01:00:52PM -0400, Jeff Layton wrote:
> > Currently the way these get set is a little convoluted. If the dentry is
> > allocated via lookup from userland, then it gets set by simple_lookup.
> > If it gets allocated when the kernel is populating the directory, then
> > it gets set via __rpc_lookup_create_exclusive, which has to check
> > whether they might already be set. Between both of these, this ensures
> > that all dentries have their d_op pointer set.
> > 
> > Instead of doing that, just have them set at d_alloc time by pointing
> > sb->s_d_op at them. With that change, we no longer want the lookup op
> > to set them, so we must move to using our own lookup routine.
> 
> There's a better solution - just make simple_lookup() skip d_set_d_op()
> if superblock already has ->s_d_op (and thus d_alloc() has already
> set the damn thing).  Voila - we can just set ->s_d_op and leave
> inode_operations as is.

Yeah, that is a better solution and the code now in mainline looks
reasonable. Trond already merged this patch however, so I'll spin up
another patch on top of mainline to convert this back to
simple_dir_inode_operations.

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>