| Message ID | 69ed4a3064bd9b48fd0593941038dd111fcfb8f3.1708709155.git.john@groves.net (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Introduce the famfs shared-memory file system | expand |
On Fri, 23 Feb 2024 11:41:46 -0600 John Groves <John@Groves.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@groves.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? :) > --- > 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? > + * > + * 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. > + * 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? > + * > + * @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? > + */ > +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) > +{ > + /* 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 > + 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. > + 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 > +#endif > void *dax_holder(struct dax_device *dax_dev); > +struct dax_device *inode_dax(struct inode *inode); Unrelated change? > 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);
On 24/02/26 12:05PM, Jonathan Cameron wrote: > On Fri, 23 Feb 2024 11:41:46 -0600 > John Groves <John@Groves.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@groves.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!
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() + * + * fs-dax file systems call this function to prepare to use a devdax device for fsdax. + * 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. + * + * @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 + */ +int fs_dax_get( + struct dax_device *dax_dev, + void *holder, + const struct dax_holder_operations *hops) +{ + /* 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)) + return -1; + + if (cmpxchg(&dax_dev->holder_data, NULL, holder)) { + pr_warn("%s: holder_data already set\n", __func__); + 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); +#endif void *dax_holder(struct dax_device *dax_dev); +struct dax_device *inode_dax(struct inode *inode); 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);
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@groves.net> --- drivers/dax/super.c | 38 ++++++++++++++++++++++++++++++++++++++ include/linux/dax.h | 5 +++++ 2 files changed, 43 insertions(+)