Re: [PATCH 26/32] afs: Use fs_context to pass parameters over automount [ver #8]

[David Howells]

Goldwyn Rodrigues  wrote:

Goldwyn Rodrigues  wrote:

> > +static struct vfsmount *afs_mntpt_do_automount(struct dentry *mntpt)
> > +{
> > +   struct fs_context *fc;
> > +   struct vfsmount *mnt;
> > +   int ret;
> > +
> > +   BUG_ON(!d_inode(mntpt));
> > +
> > +   fc = vfs_new_fs_context(_fs_type, mntpt, 0,
> > +   FS_CONTEXT_FOR_SUBMOUNT);
> > +   if (IS_ERR(fc))
> > +   return ERR_CAST(fc);
> > +
> > +   ret = afs_mntpt_set_params(fc, mntpt);
> > +   if (ret < 0)
> > +   goto error_fc;
> > +
> > +   ret = vfs_get_tree(fc);
> > +   if (ret < 0)
> > +   goto error_fc;
> > +
> > +   mnt = vfs_create_mount(fc, 0);
> > +   if (IS_ERR(mnt)) {
> > +   ret = PTR_ERR(mnt);
> > +   goto error_fc;
> > +   }
> >  
> > -   free_page((unsigned long) devname);
> > -   free_page((unsigned long) options);
> > -   _leave(" = %p", mnt);
> > +   put_fs_context(fc);
> > return mnt;
> >  
> Why are you performing a put_fs_context(fc) in the success code path? Do
> we not need a reference of fc anymore?

No.  This is the ->d_automount() hook.  The context is created at the top of
the function, configuration is done, the superblock is obtained, a mount
object is created (for which we get a ref returned).  After that point, we
don't need the context any more and no one else is going to clean it up for
us.

David

Re: [PATCH 26/32] afs: Use fs_context to pass parameters over automount [ver #8]

[David Howells]

Goldwyn Rodrigues  wrote:

Goldwyn Rodrigues  wrote:

> > +static struct vfsmount *afs_mntpt_do_automount(struct dentry *mntpt)
> > +{
> > +   struct fs_context *fc;
> > +   struct vfsmount *mnt;
> > +   int ret;
> > +
> > +   BUG_ON(!d_inode(mntpt));
> > +
> > +   fc = vfs_new_fs_context(_fs_type, mntpt, 0,
> > +   FS_CONTEXT_FOR_SUBMOUNT);
> > +   if (IS_ERR(fc))
> > +   return ERR_CAST(fc);
> > +
> > +   ret = afs_mntpt_set_params(fc, mntpt);
> > +   if (ret < 0)
> > +   goto error_fc;
> > +
> > +   ret = vfs_get_tree(fc);
> > +   if (ret < 0)
> > +   goto error_fc;
> > +
> > +   mnt = vfs_create_mount(fc, 0);
> > +   if (IS_ERR(mnt)) {
> > +   ret = PTR_ERR(mnt);
> > +   goto error_fc;
> > +   }
> >  
> > -   free_page((unsigned long) devname);
> > -   free_page((unsigned long) options);
> > -   _leave(" = %p", mnt);
> > +   put_fs_context(fc);
> > return mnt;
> >  
> Why are you performing a put_fs_context(fc) in the success code path? Do
> we not need a reference of fc anymore?

No.  This is the ->d_automount() hook.  The context is created at the top of
the function, configuration is done, the superblock is obtained, a mount
object is created (for which we get a ref returned).  After that point, we
don't need the context any more and no one else is going to clean it up for
us.

David

Re: [PATCH 26/32] afs: Use fs_context to pass parameters over automount [ver #8]

[Goldwyn Rodrigues]

On 05/24/2018 07:08 PM, David Howells wrote:
> Alter the AFS automounting code to create and modify an fs_context struct
> when parameterising a new mount triggered by an AFS mountpoint rather than
> constructing device name and option strings.
> 
> Also remove the cell=, vol= and rwpath options as they are then redundant.
> The reason they existed is because the 'device name' may be derived
> literally from a mountpoint object in the filesystem, so default cell and
> parent-type information needed to be passed in by some other method from
> the automount routines.  The vol= option didn't end up being used.
> 
> Signed-off-by: David Howells 
> cc: Eric W. Biederman 
> ---
> 
>  fs/afs/internal.h |1 
>  fs/afs/mntpt.c|  148 
> +++--
>  fs/afs/super.c|   43 +--
>  3 files changed, 79 insertions(+), 113 deletions(-)
> 
> diff --git a/fs/afs/internal.h b/fs/afs/internal.h
> index eb6e75e00181..90af5001f8c8 100644
> --- a/fs/afs/internal.h
> +++ b/fs/afs/internal.h
> @@ -35,7 +35,6 @@ struct pagevec;
>  struct afs_call;
>  
>  struct afs_fs_context {
> - boolrwpath; /* T if the parent should be 
> considered R/W */
>   boolforce;  /* T to force cell type */
>   boolautocell;   /* T if set auto mount 
> operation */
>   booldyn_root;   /* T if dynamic root */
> diff --git a/fs/afs/mntpt.c b/fs/afs/mntpt.c
> index c45aa1776591..fc383d727552 100644
> --- a/fs/afs/mntpt.c
> +++ b/fs/afs/mntpt.c
> @@ -47,6 +47,8 @@ static DECLARE_DELAYED_WORK(afs_mntpt_expiry_timer, 
> afs_mntpt_expiry_timed_out);
>  
>  static unsigned long afs_mntpt_expiry_timeout = 10 * 60;
>  
> +static const char afs_root_volume[] = "root.cell";
> +
>  /*
>   * no valid lookup procedure on this sort of dir
>   */
> @@ -68,107 +70,107 @@ static int afs_mntpt_open(struct inode *inode, struct 
> file *file)
>  }
>  
>  /*
> - * create a vfsmount to be automounted
> + * Set the parameters for the proposed superblock.
>   */
> -static struct vfsmount *afs_mntpt_do_automount(struct dentry *mntpt)
> +static int afs_mntpt_set_params(struct fs_context *fc, struct dentry *mntpt)
>  {
> - struct afs_super_info *as;
> - struct vfsmount *mnt;
> - struct afs_vnode *vnode;
> - struct page *page;
> - char *devname, *options;
> - bool rwpath = false;
> + struct afs_fs_context *ctx = fc->fs_private;
> + struct afs_vnode *vnode = AFS_FS_I(d_inode(mntpt));
> + struct afs_cell *cell;
> + const char *p;
>   int ret;
>  
> - _enter("{%pd}", mntpt);
> -
> - BUG_ON(!d_inode(mntpt));
> -
> - ret = -ENOMEM;
> - devname = (char *) get_zeroed_page(GFP_KERNEL);
> - if (!devname)
> - goto error_no_devname;
> -
> - options = (char *) get_zeroed_page(GFP_KERNEL);
> - if (!options)
> - goto error_no_options;
> -
> - vnode = AFS_FS_I(d_inode(mntpt));
>   if (test_bit(AFS_VNODE_PSEUDODIR, >flags)) {
>   /* if the directory is a pseudo directory, use the d_name */
> - static const char afs_root_cell[] = ":root.cell.";
>   unsigned size = mntpt->d_name.len;
>  
> - ret = -ENOENT;
> - if (size < 2 || size > AFS_MAXCELLNAME)
> - goto error_no_page;
> + if (size < 2)
> + return -ENOENT;
>  
> + p = mntpt->d_name.name;
>   if (mntpt->d_name.name[0] == '.') {
> - devname[0] = '%';
> - memcpy(devname + 1, mntpt->d_name.name + 1, size - 1);
> - memcpy(devname + size, afs_root_cell,
> -sizeof(afs_root_cell));
> - rwpath = true;
> - } else {
> - devname[0] = '#';
> - memcpy(devname + 1, mntpt->d_name.name, size);
> - memcpy(devname + size + 1, afs_root_cell,
> -sizeof(afs_root_cell));
> + size--;
> + p++;
> + ctx->type = AFSVL_RWVOL;
> + ctx->force = true;
> + }
> + if (size > AFS_MAXCELLNAME)
> + return -ENAMETOOLONG;
> +
> + cell = afs_lookup_cell(ctx->net, p, size, NULL, false);
> + if (IS_ERR(cell)) {
> + pr_err("kAFS: unable to lookup cell '%pd'\n", mntpt);
> + return PTR_ERR(cell);
>   }
> + afs_put_cell(ctx->net, ctx->cell);
> + ctx->cell = cell;
> +
> + ctx->volname = afs_root_volume;
> + ctx->volnamesz = sizeof(afs_root_volume) - 1;
>   } else {
>   /* read the contents of the AFS special symlink */
> + struct page *page;
>   loff_t size = i_size_read(d_inode(mntpt));

Re: [PATCH 26/32] afs: Use fs_context to pass parameters over automount [ver #8]

[Goldwyn Rodrigues]

On 05/24/2018 07:08 PM, David Howells wrote:
> Alter the AFS automounting code to create and modify an fs_context struct
> when parameterising a new mount triggered by an AFS mountpoint rather than
> constructing device name and option strings.
> 
> Also remove the cell=, vol= and rwpath options as they are then redundant.
> The reason they existed is because the 'device name' may be derived
> literally from a mountpoint object in the filesystem, so default cell and
> parent-type information needed to be passed in by some other method from
> the automount routines.  The vol= option didn't end up being used.
> 
> Signed-off-by: David Howells 
> cc: Eric W. Biederman 
> ---
> 
>  fs/afs/internal.h |1 
>  fs/afs/mntpt.c|  148 
> +++--
>  fs/afs/super.c|   43 +--
>  3 files changed, 79 insertions(+), 113 deletions(-)
> 
> diff --git a/fs/afs/internal.h b/fs/afs/internal.h
> index eb6e75e00181..90af5001f8c8 100644
> --- a/fs/afs/internal.h
> +++ b/fs/afs/internal.h
> @@ -35,7 +35,6 @@ struct pagevec;
>  struct afs_call;
>  
>  struct afs_fs_context {
> - boolrwpath; /* T if the parent should be 
> considered R/W */
>   boolforce;  /* T to force cell type */
>   boolautocell;   /* T if set auto mount 
> operation */
>   booldyn_root;   /* T if dynamic root */
> diff --git a/fs/afs/mntpt.c b/fs/afs/mntpt.c
> index c45aa1776591..fc383d727552 100644
> --- a/fs/afs/mntpt.c
> +++ b/fs/afs/mntpt.c
> @@ -47,6 +47,8 @@ static DECLARE_DELAYED_WORK(afs_mntpt_expiry_timer, 
> afs_mntpt_expiry_timed_out);
>  
>  static unsigned long afs_mntpt_expiry_timeout = 10 * 60;
>  
> +static const char afs_root_volume[] = "root.cell";
> +
>  /*
>   * no valid lookup procedure on this sort of dir
>   */
> @@ -68,107 +70,107 @@ static int afs_mntpt_open(struct inode *inode, struct 
> file *file)
>  }
>  
>  /*
> - * create a vfsmount to be automounted
> + * Set the parameters for the proposed superblock.
>   */
> -static struct vfsmount *afs_mntpt_do_automount(struct dentry *mntpt)
> +static int afs_mntpt_set_params(struct fs_context *fc, struct dentry *mntpt)
>  {
> - struct afs_super_info *as;
> - struct vfsmount *mnt;
> - struct afs_vnode *vnode;
> - struct page *page;
> - char *devname, *options;
> - bool rwpath = false;
> + struct afs_fs_context *ctx = fc->fs_private;
> + struct afs_vnode *vnode = AFS_FS_I(d_inode(mntpt));
> + struct afs_cell *cell;
> + const char *p;
>   int ret;
>  
> - _enter("{%pd}", mntpt);
> -
> - BUG_ON(!d_inode(mntpt));
> -
> - ret = -ENOMEM;
> - devname = (char *) get_zeroed_page(GFP_KERNEL);
> - if (!devname)
> - goto error_no_devname;
> -
> - options = (char *) get_zeroed_page(GFP_KERNEL);
> - if (!options)
> - goto error_no_options;
> -
> - vnode = AFS_FS_I(d_inode(mntpt));
>   if (test_bit(AFS_VNODE_PSEUDODIR, >flags)) {
>   /* if the directory is a pseudo directory, use the d_name */
> - static const char afs_root_cell[] = ":root.cell.";
>   unsigned size = mntpt->d_name.len;
>  
> - ret = -ENOENT;
> - if (size < 2 || size > AFS_MAXCELLNAME)
> - goto error_no_page;
> + if (size < 2)
> + return -ENOENT;
>  
> + p = mntpt->d_name.name;
>   if (mntpt->d_name.name[0] == '.') {
> - devname[0] = '%';
> - memcpy(devname + 1, mntpt->d_name.name + 1, size - 1);
> - memcpy(devname + size, afs_root_cell,
> -sizeof(afs_root_cell));
> - rwpath = true;
> - } else {
> - devname[0] = '#';
> - memcpy(devname + 1, mntpt->d_name.name, size);
> - memcpy(devname + size + 1, afs_root_cell,
> -sizeof(afs_root_cell));
> + size--;
> + p++;
> + ctx->type = AFSVL_RWVOL;
> + ctx->force = true;
> + }
> + if (size > AFS_MAXCELLNAME)
> + return -ENAMETOOLONG;
> +
> + cell = afs_lookup_cell(ctx->net, p, size, NULL, false);
> + if (IS_ERR(cell)) {
> + pr_err("kAFS: unable to lookup cell '%pd'\n", mntpt);
> + return PTR_ERR(cell);
>   }
> + afs_put_cell(ctx->net, ctx->cell);
> + ctx->cell = cell;
> +
> + ctx->volname = afs_root_volume;
> + ctx->volnamesz = sizeof(afs_root_volume) - 1;
>   } else {
>   /* read the contents of the AFS special symlink */
> + struct page *page;
>   loff_t size = i_size_read(d_inode(mntpt));

[PATCH 26/32] afs: Use fs_context to pass parameters over automount [ver #8]

[David Howells]

Alter the AFS automounting code to create and modify an fs_context struct
when parameterising a new mount triggered by an AFS mountpoint rather than
constructing device name and option strings.

Also remove the cell=, vol= and rwpath options as they are then redundant.
The reason they existed is because the 'device name' may be derived
literally from a mountpoint object in the filesystem, so default cell and
parent-type information needed to be passed in by some other method from
the automount routines.  The vol= option didn't end up being used.

Signed-off-by: David Howells 
cc: Eric W. Biederman 
---

 fs/afs/internal.h |1 
 fs/afs/mntpt.c|  148 +++--
 fs/afs/super.c|   43 +--
 3 files changed, 79 insertions(+), 113 deletions(-)

diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index eb6e75e00181..90af5001f8c8 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -35,7 +35,6 @@ struct pagevec;
 struct afs_call;
 
 struct afs_fs_context {
-   boolrwpath; /* T if the parent should be 
considered R/W */
boolforce;  /* T to force cell type */
boolautocell;   /* T if set auto mount 
operation */
booldyn_root;   /* T if dynamic root */
diff --git a/fs/afs/mntpt.c b/fs/afs/mntpt.c
index c45aa1776591..fc383d727552 100644
--- a/fs/afs/mntpt.c
+++ b/fs/afs/mntpt.c
@@ -47,6 +47,8 @@ static DECLARE_DELAYED_WORK(afs_mntpt_expiry_timer, 
afs_mntpt_expiry_timed_out);
 
 static unsigned long afs_mntpt_expiry_timeout = 10 * 60;
 
+static const char afs_root_volume[] = "root.cell";
+
 /*
  * no valid lookup procedure on this sort of dir
  */
@@ -68,107 +70,107 @@ static int afs_mntpt_open(struct inode *inode, struct 
file *file)
 }
 
 /*
- * create a vfsmount to be automounted
+ * Set the parameters for the proposed superblock.
  */
-static struct vfsmount *afs_mntpt_do_automount(struct dentry *mntpt)
+static int afs_mntpt_set_params(struct fs_context *fc, struct dentry *mntpt)
 {
-   struct afs_super_info *as;
-   struct vfsmount *mnt;
-   struct afs_vnode *vnode;
-   struct page *page;
-   char *devname, *options;
-   bool rwpath = false;
+   struct afs_fs_context *ctx = fc->fs_private;
+   struct afs_vnode *vnode = AFS_FS_I(d_inode(mntpt));
+   struct afs_cell *cell;
+   const char *p;
int ret;
 
-   _enter("{%pd}", mntpt);
-
-   BUG_ON(!d_inode(mntpt));
-
-   ret = -ENOMEM;
-   devname = (char *) get_zeroed_page(GFP_KERNEL);
-   if (!devname)
-   goto error_no_devname;
-
-   options = (char *) get_zeroed_page(GFP_KERNEL);
-   if (!options)
-   goto error_no_options;
-
-   vnode = AFS_FS_I(d_inode(mntpt));
if (test_bit(AFS_VNODE_PSEUDODIR, >flags)) {
/* if the directory is a pseudo directory, use the d_name */
-   static const char afs_root_cell[] = ":root.cell.";
unsigned size = mntpt->d_name.len;
 
-   ret = -ENOENT;
-   if (size < 2 || size > AFS_MAXCELLNAME)
-   goto error_no_page;
+   if (size < 2)
+   return -ENOENT;
 
+   p = mntpt->d_name.name;
if (mntpt->d_name.name[0] == '.') {
-   devname[0] = '%';
-   memcpy(devname + 1, mntpt->d_name.name + 1, size - 1);
-   memcpy(devname + size, afs_root_cell,
-  sizeof(afs_root_cell));
-   rwpath = true;
-   } else {
-   devname[0] = '#';
-   memcpy(devname + 1, mntpt->d_name.name, size);
-   memcpy(devname + size + 1, afs_root_cell,
-  sizeof(afs_root_cell));
+   size--;
+   p++;
+   ctx->type = AFSVL_RWVOL;
+   ctx->force = true;
+   }
+   if (size > AFS_MAXCELLNAME)
+   return -ENAMETOOLONG;
+
+   cell = afs_lookup_cell(ctx->net, p, size, NULL, false);
+   if (IS_ERR(cell)) {
+   pr_err("kAFS: unable to lookup cell '%pd'\n", mntpt);
+   return PTR_ERR(cell);
}
+   afs_put_cell(ctx->net, ctx->cell);
+   ctx->cell = cell;
+
+   ctx->volname = afs_root_volume;
+   ctx->volnamesz = sizeof(afs_root_volume) - 1;
} else {
/* read the contents of the AFS special symlink */
+   struct page *page;
loff_t size = i_size_read(d_inode(mntpt));
char *buf;
 
-   ret = -EINVAL;
if (size > PAGE_SIZE - 1)
-   goto 

[PATCH 26/32] afs: Use fs_context to pass parameters over automount [ver #8]

[David Howells]

Alter the AFS automounting code to create and modify an fs_context struct
when parameterising a new mount triggered by an AFS mountpoint rather than
constructing device name and option strings.

Also remove the cell=, vol= and rwpath options as they are then redundant.
The reason they existed is because the 'device name' may be derived
literally from a mountpoint object in the filesystem, so default cell and
parent-type information needed to be passed in by some other method from
the automount routines.  The vol= option didn't end up being used.

Signed-off-by: David Howells 
cc: Eric W. Biederman 
---

 fs/afs/internal.h |1 
 fs/afs/mntpt.c|  148 +++--
 fs/afs/super.c|   43 +--
 3 files changed, 79 insertions(+), 113 deletions(-)

diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index eb6e75e00181..90af5001f8c8 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -35,7 +35,6 @@ struct pagevec;
 struct afs_call;
 
 struct afs_fs_context {
-   boolrwpath; /* T if the parent should be 
considered R/W */
boolforce;  /* T to force cell type */
boolautocell;   /* T if set auto mount 
operation */
booldyn_root;   /* T if dynamic root */
diff --git a/fs/afs/mntpt.c b/fs/afs/mntpt.c
index c45aa1776591..fc383d727552 100644
--- a/fs/afs/mntpt.c
+++ b/fs/afs/mntpt.c
@@ -47,6 +47,8 @@ static DECLARE_DELAYED_WORK(afs_mntpt_expiry_timer, 
afs_mntpt_expiry_timed_out);
 
 static unsigned long afs_mntpt_expiry_timeout = 10 * 60;
 
+static const char afs_root_volume[] = "root.cell";
+
 /*
  * no valid lookup procedure on this sort of dir
  */
@@ -68,107 +70,107 @@ static int afs_mntpt_open(struct inode *inode, struct 
file *file)
 }
 
 /*
- * create a vfsmount to be automounted
+ * Set the parameters for the proposed superblock.
  */
-static struct vfsmount *afs_mntpt_do_automount(struct dentry *mntpt)
+static int afs_mntpt_set_params(struct fs_context *fc, struct dentry *mntpt)
 {
-   struct afs_super_info *as;
-   struct vfsmount *mnt;
-   struct afs_vnode *vnode;
-   struct page *page;
-   char *devname, *options;
-   bool rwpath = false;
+   struct afs_fs_context *ctx = fc->fs_private;
+   struct afs_vnode *vnode = AFS_FS_I(d_inode(mntpt));
+   struct afs_cell *cell;
+   const char *p;
int ret;
 
-   _enter("{%pd}", mntpt);
-
-   BUG_ON(!d_inode(mntpt));
-
-   ret = -ENOMEM;
-   devname = (char *) get_zeroed_page(GFP_KERNEL);
-   if (!devname)
-   goto error_no_devname;
-
-   options = (char *) get_zeroed_page(GFP_KERNEL);
-   if (!options)
-   goto error_no_options;
-
-   vnode = AFS_FS_I(d_inode(mntpt));
if (test_bit(AFS_VNODE_PSEUDODIR, >flags)) {
/* if the directory is a pseudo directory, use the d_name */
-   static const char afs_root_cell[] = ":root.cell.";
unsigned size = mntpt->d_name.len;
 
-   ret = -ENOENT;
-   if (size < 2 || size > AFS_MAXCELLNAME)
-   goto error_no_page;
+   if (size < 2)
+   return -ENOENT;
 
+   p = mntpt->d_name.name;
if (mntpt->d_name.name[0] == '.') {
-   devname[0] = '%';
-   memcpy(devname + 1, mntpt->d_name.name + 1, size - 1);
-   memcpy(devname + size, afs_root_cell,
-  sizeof(afs_root_cell));
-   rwpath = true;
-   } else {
-   devname[0] = '#';
-   memcpy(devname + 1, mntpt->d_name.name, size);
-   memcpy(devname + size + 1, afs_root_cell,
-  sizeof(afs_root_cell));
+   size--;
+   p++;
+   ctx->type = AFSVL_RWVOL;
+   ctx->force = true;
+   }
+   if (size > AFS_MAXCELLNAME)
+   return -ENAMETOOLONG;
+
+   cell = afs_lookup_cell(ctx->net, p, size, NULL, false);
+   if (IS_ERR(cell)) {
+   pr_err("kAFS: unable to lookup cell '%pd'\n", mntpt);
+   return PTR_ERR(cell);
}
+   afs_put_cell(ctx->net, ctx->cell);
+   ctx->cell = cell;
+
+   ctx->volname = afs_root_volume;
+   ctx->volnamesz = sizeof(afs_root_volume) - 1;
} else {
/* read the contents of the AFS special symlink */
+   struct page *page;
loff_t size = i_size_read(d_inode(mntpt));
char *buf;
 
-   ret = -EINVAL;
if (size > PAGE_SIZE - 1)
-   goto error_no_page;
+   return