diff mbox series

[RFC,18/20] famfs: Support character dax via the dev_dax_iomap patch

Message ID fa06095b6a05a26a0a016768b2e2b70663163eeb.1708709155.git.john@groves.net
State Superseded
Headers show
Series Introduce the famfs shared-memory file system | expand

Commit Message

John Groves Feb. 23, 2024, 5:42 p.m. UTC
This commit introduces the ability to open a character /dev/dax device
instead of a block /dev/pmem device. This rests on the dev_dax_iomap
patches earlier in this series.

Signed-off-by: John Groves <john@groves.net>
---
 fs/famfs/famfs_inode.c | 97 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 87 insertions(+), 10 deletions(-)

Comments

Jonathan Cameron Feb. 26, 2024, 1:52 p.m. UTC | #1
On Fri, 23 Feb 2024 11:42:02 -0600
John Groves <John@Groves.net> wrote:

> This commit introduces the ability to open a character /dev/dax device
> instead of a block /dev/pmem device. This rests on the dev_dax_iomap
> patches earlier in this series.

Not sure the back reference is needed given it's in the series.

> 
> Signed-off-by: John Groves <john@groves.net>
> ---
>  fs/famfs/famfs_inode.c | 97 +++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 87 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/famfs/famfs_inode.c b/fs/famfs/famfs_inode.c
> index 0d659820e8ff..7d65ac497147 100644
> --- a/fs/famfs/famfs_inode.c
> +++ b/fs/famfs/famfs_inode.c
> @@ -215,6 +215,93 @@ static const struct super_operations famfs_ops = {
>  	.show_options	= famfs_show_options,
>  };
>  
> +/*****************************************************************************/
> +
> +#if defined(CONFIG_DEV_DAX_IOMAP)
> +
> +/*
> + * famfs dax_operations  (for char dax)
> + */
> +static int
> +famfs_dax_notify_failure(struct dax_device *dax_dev, u64 offset,
> +			u64 len, int mf_flags)
> +{
> +	pr_err("%s: offset %lld len %llu flags %x\n", __func__,
> +	       offset, len, mf_flags);
> +	return -EOPNOTSUPP;
> +}
> +
> +static const struct dax_holder_operations famfs_dax_holder_ops = {
> +	.notify_failure		= famfs_dax_notify_failure,
> +};
> +
> +/*****************************************************************************/
> +
> +/**
> + * famfs_open_char_device()
> + *
> + * Open a /dev/dax device. This only works in kernels with the dev_dax_iomap patch

That comment you definitely don't need as this won't get merged without
that patch being in place.


> + */
> +static int
> +famfs_open_char_device(
> +	struct super_block *sb,
> +	struct fs_context  *fc)
> +{
> +	struct famfs_fs_info *fsi = sb->s_fs_info;
> +	struct dax_device    *dax_devp;
> +	struct inode         *daxdev_inode;
> +
> +	int rc = 0;
set in all paths where it's used.

> +
> +	pr_notice("%s: Opening character dax device %s\n", __func__, fc->source);

pr_debug

> +
> +	fsi->dax_filp = filp_open(fc->source, O_RDWR, 0);
> +	if (IS_ERR(fsi->dax_filp)) {
> +		pr_err("%s: failed to open dax device %s\n",
> +		       __func__, fc->source);
> +		fsi->dax_filp = NULL;
Better to use a local variable

	fp = filp_open(fc->source, O_RDWR, 0);
	if (IS_ERR(fp)) {
		pr_err.
		return;
	}
	fsi->dax_filp = fp;
or similar.

> +		return PTR_ERR(fsi->dax_filp);
> +	}
> +
> +	daxdev_inode = file_inode(fsi->dax_filp);
> +	dax_devp     = inode_dax(daxdev_inode);
> +	if (IS_ERR(dax_devp)) {
> +		pr_err("%s: unable to get daxdev from inode for %s\n",
> +		       __func__, fc->source);
> +		rc = -ENODEV;
> +		goto char_err;
> +	}
> +
> +	rc = fs_dax_get(dax_devp, fsi, &famfs_dax_holder_ops);
> +	if (rc) {
> +		pr_info("%s: err attaching famfs_dax_holder_ops\n", __func__);
> +		goto char_err;
> +	}
> +
> +	fsi->bdev_handle = NULL;
> +	fsi->dax_devp = dax_devp;
> +
> +	return 0;
> +
> +char_err:
> +	filp_close(fsi->dax_filp, NULL);

You carefully set fsi->dax_filp to null in other other error paths.
Why there and not here?

> +	return rc;
> +}
> +
> +#else /* CONFIG_DEV_DAX_IOMAP */
> +static int
> +famfs_open_char_device(
> +	struct super_block *sb,
> +	struct fs_context  *fc)
> +{
> +	pr_err("%s: Root device is %s, but your kernel does not support famfs on /dev/dax\n",
> +	       __func__, fc->source);
> +	return -ENODEV;
> +}
> +
> +
> +#endif /* CONFIG_DEV_DAX_IOMAP */
> +
>  /***************************************************************************************
>   * dax_holder_operations for block dax
>   */
> @@ -236,16 +323,6 @@ const struct dax_holder_operations famfs_blk_dax_holder_ops = {
>  	.notify_failure		= famfs_blk_dax_notify_failure,
>  };
>  

Put it in right place earlier! Makes this less noisy.

> -static int
> -famfs_open_char_device(
> -	struct super_block *sb,
> -	struct fs_context  *fc)
> -{
> -	pr_err("%s: Root device is %s, but your kernel does not support famfs on /dev/dax\n",
> -	       __func__, fc->source);
> -	return -ENODEV;
> -}
> -
>  /**
>   * famfs_open_device()
>   *
John Groves Feb. 27, 2024, 10:27 p.m. UTC | #2
On 24/02/26 01:52PM, Jonathan Cameron wrote:
> On Fri, 23 Feb 2024 11:42:02 -0600
> John Groves <John@Groves.net> wrote:
> 
> > This commit introduces the ability to open a character /dev/dax device
> > instead of a block /dev/pmem device. This rests on the dev_dax_iomap
> > patches earlier in this series.
> 
> Not sure the back reference is needed given it's in the series.

Roger that

> 
> > 
> > Signed-off-by: John Groves <john@groves.net>
> > ---
> >  fs/famfs/famfs_inode.c | 97 +++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 87 insertions(+), 10 deletions(-)
> > 
> > diff --git a/fs/famfs/famfs_inode.c b/fs/famfs/famfs_inode.c
> > index 0d659820e8ff..7d65ac497147 100644
> > --- a/fs/famfs/famfs_inode.c
> > +++ b/fs/famfs/famfs_inode.c
> > @@ -215,6 +215,93 @@ static const struct super_operations famfs_ops = {
> >  	.show_options	= famfs_show_options,
> >  };
> >  
> > +/*****************************************************************************/
> > +
> > +#if defined(CONFIG_DEV_DAX_IOMAP)
> > +
> > +/*
> > + * famfs dax_operations  (for char dax)
> > + */
> > +static int
> > +famfs_dax_notify_failure(struct dax_device *dax_dev, u64 offset,
> > +			u64 len, int mf_flags)
> > +{
> > +	pr_err("%s: offset %lld len %llu flags %x\n", __func__,
> > +	       offset, len, mf_flags);
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > +static const struct dax_holder_operations famfs_dax_holder_ops = {
> > +	.notify_failure		= famfs_dax_notify_failure,
> > +};
> > +
> > +/*****************************************************************************/
> > +
> > +/**
> > + * famfs_open_char_device()
> > + *
> > + * Open a /dev/dax device. This only works in kernels with the dev_dax_iomap patch
> 
> That comment you definitely don't need as this won't get merged without
> that patch being in place.

This will be gone from v2. I'm 90% sure there is no reason to keep the block
device backing support (pmem), since devdax is the point AND pmem can be
converted to devdax mode. So famfs will become devdax only...etc.

This was under development for quite a few months, and actually working,
I got the dev_dax_iomap right (er, "right enough" for it to work :D). But now
that dev_dax_iomap looks basically stable, pmem/block support can come out.

> 
> 
> > + */
> > +static int
> > +famfs_open_char_device(
> > +	struct super_block *sb,
> > +	struct fs_context  *fc)
> > +{
> > +	struct famfs_fs_info *fsi = sb->s_fs_info;
> > +	struct dax_device    *dax_devp;
> > +	struct inode         *daxdev_inode;
> > +
> > +	int rc = 0;
> set in all paths where it's used.
> 
> > +
> > +	pr_notice("%s: Opening character dax device %s\n", __func__, fc->source);
> 
> pr_debug

Done

> 
> > +
> > +	fsi->dax_filp = filp_open(fc->source, O_RDWR, 0);
> > +	if (IS_ERR(fsi->dax_filp)) {
> > +		pr_err("%s: failed to open dax device %s\n",
> > +		       __func__, fc->source);
> > +		fsi->dax_filp = NULL;
> Better to use a local variable
> 
> 	fp = filp_open(fc->source, O_RDWR, 0);
> 	if (IS_ERR(fp)) {
> 		pr_err.
> 		return;
> 	}
> 	fsi->dax_filp = fp;
> or similar.

Done, thanks.

> 
> > +		return PTR_ERR(fsi->dax_filp);
> > +	}
> > +
> > +	daxdev_inode = file_inode(fsi->dax_filp);
> > +	dax_devp     = inode_dax(daxdev_inode);
> > +	if (IS_ERR(dax_devp)) {
> > +		pr_err("%s: unable to get daxdev from inode for %s\n",
> > +		       __func__, fc->source);
> > +		rc = -ENODEV;
> > +		goto char_err;
> > +	}
> > +
> > +	rc = fs_dax_get(dax_devp, fsi, &famfs_dax_holder_ops);
> > +	if (rc) {
> > +		pr_info("%s: err attaching famfs_dax_holder_ops\n", __func__);
> > +		goto char_err;
> > +	}
> > +
> > +	fsi->bdev_handle = NULL;
> > +	fsi->dax_devp = dax_devp;
> > +
> > +	return 0;
> > +
> > +char_err:
> > +	filp_close(fsi->dax_filp, NULL);
> 
> You carefully set fsi->dax_filp to null in other other error paths.
> Why there and not here?

Why indeed - done now.

> 
> > +	return rc;
> > +}
> > +
> > +#else /* CONFIG_DEV_DAX_IOMAP */
> > +static int
> > +famfs_open_char_device(
> > +	struct super_block *sb,
> > +	struct fs_context  *fc)
> > +{
> > +	pr_err("%s: Root device is %s, but your kernel does not support famfs on /dev/dax\n",
> > +	       __func__, fc->source);
> > +	return -ENODEV;
> > +}
> > +
> > +
> > +#endif /* CONFIG_DEV_DAX_IOMAP */
> > +
> >  /***************************************************************************************
> >   * dax_holder_operations for block dax
> >   */
> > @@ -236,16 +323,6 @@ const struct dax_holder_operations famfs_blk_dax_holder_ops = {
> >  	.notify_failure		= famfs_blk_dax_notify_failure,
> >  };
> >  
> 
> Put it in right place earlier! Makes this less noisy.

This will be eliminated by the move to /dev/dax-only

Thanks,
John
diff mbox series

Patch

diff --git a/fs/famfs/famfs_inode.c b/fs/famfs/famfs_inode.c
index 0d659820e8ff..7d65ac497147 100644
--- a/fs/famfs/famfs_inode.c
+++ b/fs/famfs/famfs_inode.c
@@ -215,6 +215,93 @@  static const struct super_operations famfs_ops = {
 	.show_options	= famfs_show_options,
 };
 
+/*****************************************************************************/
+
+#if defined(CONFIG_DEV_DAX_IOMAP)
+
+/*
+ * famfs dax_operations  (for char dax)
+ */
+static int
+famfs_dax_notify_failure(struct dax_device *dax_dev, u64 offset,
+			u64 len, int mf_flags)
+{
+	pr_err("%s: offset %lld len %llu flags %x\n", __func__,
+	       offset, len, mf_flags);
+	return -EOPNOTSUPP;
+}
+
+static const struct dax_holder_operations famfs_dax_holder_ops = {
+	.notify_failure		= famfs_dax_notify_failure,
+};
+
+/*****************************************************************************/
+
+/**
+ * famfs_open_char_device()
+ *
+ * Open a /dev/dax device. This only works in kernels with the dev_dax_iomap patch
+ */
+static int
+famfs_open_char_device(
+	struct super_block *sb,
+	struct fs_context  *fc)
+{
+	struct famfs_fs_info *fsi = sb->s_fs_info;
+	struct dax_device    *dax_devp;
+	struct inode         *daxdev_inode;
+
+	int rc = 0;
+
+	pr_notice("%s: Opening character dax device %s\n", __func__, fc->source);
+
+	fsi->dax_filp = filp_open(fc->source, O_RDWR, 0);
+	if (IS_ERR(fsi->dax_filp)) {
+		pr_err("%s: failed to open dax device %s\n",
+		       __func__, fc->source);
+		fsi->dax_filp = NULL;
+		return PTR_ERR(fsi->dax_filp);
+	}
+
+	daxdev_inode = file_inode(fsi->dax_filp);
+	dax_devp     = inode_dax(daxdev_inode);
+	if (IS_ERR(dax_devp)) {
+		pr_err("%s: unable to get daxdev from inode for %s\n",
+		       __func__, fc->source);
+		rc = -ENODEV;
+		goto char_err;
+	}
+
+	rc = fs_dax_get(dax_devp, fsi, &famfs_dax_holder_ops);
+	if (rc) {
+		pr_info("%s: err attaching famfs_dax_holder_ops\n", __func__);
+		goto char_err;
+	}
+
+	fsi->bdev_handle = NULL;
+	fsi->dax_devp = dax_devp;
+
+	return 0;
+
+char_err:
+	filp_close(fsi->dax_filp, NULL);
+	return rc;
+}
+
+#else /* CONFIG_DEV_DAX_IOMAP */
+static int
+famfs_open_char_device(
+	struct super_block *sb,
+	struct fs_context  *fc)
+{
+	pr_err("%s: Root device is %s, but your kernel does not support famfs on /dev/dax\n",
+	       __func__, fc->source);
+	return -ENODEV;
+}
+
+
+#endif /* CONFIG_DEV_DAX_IOMAP */
+
 /***************************************************************************************
  * dax_holder_operations for block dax
  */
@@ -236,16 +323,6 @@  const struct dax_holder_operations famfs_blk_dax_holder_ops = {
 	.notify_failure		= famfs_blk_dax_notify_failure,
 };
 
-static int
-famfs_open_char_device(
-	struct super_block *sb,
-	struct fs_context  *fc)
-{
-	pr_err("%s: Root device is %s, but your kernel does not support famfs on /dev/dax\n",
-	       __func__, fc->source);
-	return -ENODEV;
-}
-
 /**
  * famfs_open_device()
  *