Message-ID: <7t6n6c4cycu2hqh4ajsl4egtu2womq54unj4ikqeu3rehmxwzw@64ydmjh5x2ga>
Date: Mon, 26 Feb 2024 09:00:31 -0600
From: John Groves <John@...ves.net>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: John Groves <jgroves@...ron.com>, Jonathan Corbet <corbet@....net>, 
	Dan Williams <dan.j.williams@...el.com>, Vishal Verma <vishal.l.verma@...el.com>, 
	Dave Jiang <dave.jiang@...el.com>, Alexander Viro <viro@...iv.linux.org.uk>, 
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>, Matthew Wilcox <willy@...radead.org>, 
	linux-cxl@...r.kernel.org, linux-fsdevel@...r.kernel.org, linux-doc@...r.kernel.org, 
	linux-kernel@...r.kernel.org, nvdimm@...ts.linux.dev, john@...alactic.com, 
	Dave Chinner <david@...morbit.com>, Christoph Hellwig <hch@...radead.org>, 
	dave.hansen@...ux.intel.com, gregory.price@...verge.com
Subject: Re: [RFC PATCH 02/20] dev_dax_iomap: Add fs_dax_get() func to
 prepare dax for fs-dax usage

On 24/02/26 12:05PM, Jonathan Cameron wrote:
> On Fri, 23 Feb 2024 11:41:46 -0600
> John Groves <John@...ves.net> wrote:
> 
> > This function should be called by fs-dax file systems after opening the
> > devdax device. This adds holder_operations.
> > 
> > This function serves the same role as fs_dax_get_by_bdev(), which dax
> > file systems call after opening the pmem block device.
> > 
> > Signed-off-by: John Groves <john@...ves.net>
> 
> A few trivial comments form a first read to get my head around this.
> 
> Yeah, it is only an RFC, but who doesn't like tidy code? :)

Hope your eyes don't burn too much ;)
> 
> 
> > ---
> >  drivers/dax/super.c | 38 ++++++++++++++++++++++++++++++++++++++
> >  include/linux/dax.h |  5 +++++
> >  2 files changed, 43 insertions(+)
> > 
> > diff --git a/drivers/dax/super.c b/drivers/dax/super.c
> > index f4b635526345..fc96362de237 100644
> > --- a/drivers/dax/super.c
> > +++ b/drivers/dax/super.c
> > @@ -121,6 +121,44 @@ void fs_put_dax(struct dax_device *dax_dev, void *holder)
> >  EXPORT_SYMBOL_GPL(fs_put_dax);
> >  #endif /* CONFIG_BLOCK && CONFIG_FS_DAX */
> >  
> > +#if IS_ENABLED(CONFIG_DEV_DAX_IOMAP)
> > +
> > +/**
> > + * fs_dax_get()
> 
> Smells like kernel doc but fairly sure it needs a short description.
> Have you sanity checked for warnings when running scripts/kerneldoc on it?

Right, and there were other cases. Randy pointed one out, and I've already
gone through and "fixed" them.

> 
> > + *
> > + * fs-dax file systems call this function to prepare to use a devdax device for fsdax.
> Trivial but lines too long. Keep under 80 chars unless there is a strong
> readability arguement for not doing so.

I was under the impression the "kids these days" have a 100 column standard.
But I will go through and limit line to 80 except where it gets too awkward.

> 
> 
> > + * This is like fs_dax_get_by_bdev(), but the caller already has struct dev_dax (and there
> > + * is no bdev). The holder makes this exclusive.
> 
> Not familiar with this area: what does exclusive mean here?

The holder_ops are set via cmpxchg, in such a way that if there are already
holder_ops, the call to fs_dax_get() will fail. (as it should)

> 
> > + *
> > + * @dax_dev: dev to be prepared for fs-dax usage
> > + * @holder: filesystem or mapped device inside the dax_device
> > + * @hops: operations for the inner holder
> > + *
> > + * Returns: 0 on success, -1 on failure
> 
> Why not return < 0 and use somewhat useful return values?

Good idea, will do.

> 
> > + */
> > +int fs_dax_get(
> > +	struct dax_device *dax_dev,
> > +	void *holder,
> > +	const struct dax_holder_operations *hops)
> 
> Match local style for indents - it's a bit inconsistent but probably...
> 
> int fs_dax_get(struct dad_device *dev_dax, void *holder,
> 	       const struct dax_holder_operations *hops)

Done

> 
> > +{
> > +	/* dax_dev->ops should have been populated by devm_create_dev_dax() */
> > +	if (WARN_ON(!dax_dev->ops))
> > +		return -1;
> > +
> > +	if (!dax_dev || !dax_alive(dax_dev) || !igrab(&dax_dev->inode))
> 
> You dereferenced dax_dev on the line above so check is too late or
> unnecessary

Good catch, thank you!

> 
> > +		return -1;
> > +
> > +	if (cmpxchg(&dax_dev->holder_data, NULL, holder)) {
> > +		pr_warn("%s: holder_data already set\n", __func__);
> 
> Perhaps nicer to use a pr_fmt() deal with the func name if you need it.
> or make it pr_debug and let dynamic debug control formatting if anyone
> wants the function name.

Sounds good.

> 
> > +		return -1;
> > +	}
> > +	dax_dev->holder_ops = hops;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(fs_dax_get);
> > +#endif /* DEV_DAX_IOMAP */
> > +
> >  enum dax_device_flags {
> >  	/* !alive + rcu grace period == no new operations / mappings */
> >  	DAXDEV_ALIVE,
> > diff --git a/include/linux/dax.h b/include/linux/dax.h
> > index b463502b16e1..e973289bfde3 100644
> > --- a/include/linux/dax.h
> > +++ b/include/linux/dax.h
> > @@ -57,7 +57,12 @@ struct dax_holder_operations {
> >  
> >  #if IS_ENABLED(CONFIG_DAX)
> >  struct dax_device *alloc_dax(void *private, const struct dax_operations *ops);
> > +
> > +#if IS_ENABLED(CONFIG_DEV_DAX_IOMAP)
> > +int fs_dax_get(struct dax_device *dax_dev, void *holder, const struct dax_holder_operations *hops);
> line wrap < 80 chars

Roger that

> 
> > +#endif
> >  void *dax_holder(struct dax_device *dax_dev);
> > +struct dax_device *inode_dax(struct inode *inode);
> 
> Unrelated change?

Kinda, but I'm not sure there is a better home for this one. Patch 18,
which is a famfs patch, calls inode_dax(). It was already exported but not
prototyped in dax.h.

Mixing it in with other dev_dax_iomap content seems better than mixing it
with famfs content. Could make it a separate patch, but I was trying to
some old docs that said keep patch sets <=15 - which I deemed impossible here.

What say others?

> 
> >  void put_dax(struct dax_device *dax_dev);
> >  void kill_dax(struct dax_device *dax_dev);
> >  void dax_write_cache(struct dax_device *dax_dev, bool wc);
> 

Thanks Jonathan!

Powered by blists - more mailing lists