Received-SPF: pass (google.com: domain of linux-kernel+bounces-84127-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Sender: John Groves <grovesaustin@gmail.com>
Date: Tue, 27 Feb 2024 16:27:06 -0600
From: John Groves <John@groves.net>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: John Groves <jgroves@micron.com>, Jonathan Corbet <corbet@lwn.net>, 
	Dan Williams <dan.j.williams@intel.com>, Vishal Verma <vishal.l.verma@intel.com>, 
	Dave Jiang <dave.jiang@intel.com>, Alexander Viro <viro@zeniv.linux.org.uk>, 
	Christian Brauner <brauner@kernel.org>, Jan Kara <jack@suse.cz>, Matthew Wilcox <willy@infradead.org>, 
	linux-cxl@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-doc@vger.kernel.org, 
	linux-kernel@vger.kernel.org, nvdimm@lists.linux.dev, john@jagalactic.com, 
	Dave Chinner <david@fromorbit.com>, Christoph Hellwig <hch@infradead.org>, 
	dave.hansen@linux.intel.com, gregory.price@memverge.com
Subject: Re: [RFC PATCH 18/20] famfs: Support character dax via the
 dev_dax_iomap patch
Message-ID: <iwcbiag4hrcbbpujd6xqatfq77oxdab7a7eeqbstg64u62edad@5bfru25qxz37>
References: <cover.1708709155.git.john@groves.net>
 <fa06095b6a05a26a0a016768b2e2b70663163eeb.1708709155.git.john@groves.net>
 <20240226135228.00007714@Huawei.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240226135228.00007714@Huawei.com>

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