[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 2 Feb 2024 09:40:20 -0500
From: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To: Dan Williams <dan.j.williams@...el.com>, Arnd Bergmann <arnd@...db.de>,
Dave Chinner <david@...morbit.com>
Cc: linux-kernel@...r.kernel.org, Andrew Morton <akpm@...ux-foundation.org>,
Linus Torvalds <torvalds@...ux-foundation.org>, linux-mm@...ck.org,
linux-arch@...r.kernel.org, Vishal Verma <vishal.l.verma@...el.com>,
Dave Jiang <dave.jiang@...el.com>, Matthew Wilcox <willy@...radead.org>,
Russell King <linux@...linux.org.uk>, nvdimm@...ts.linux.dev,
linux-cxl@...r.kernel.org, linux-fsdevel@...r.kernel.org,
dm-devel@...ts.linux.dev
Subject: Re: [RFC PATCH v3 2/4] dax: Check for data cache aliasing at runtime
On 2024-02-01 10:44, Mathieu Desnoyers wrote:
[...]
>> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
>> index 4e8fdcb3f1c8..b69c9e442cf4 100644
>> --- a/drivers/nvdimm/pmem.c
>> +++ b/drivers/nvdimm/pmem.c
>> @@ -560,17 +560,19 @@ static int pmem_attach_disk(struct device *dev,
>> dax_dev = alloc_dax(pmem, &pmem_dax_ops);
>> if (IS_ERR(dax_dev)) {
>> rc = PTR_ERR(dax_dev);
>> - goto out;
>> + if (rc != -EOPNOTSUPP)
>> + goto out;
>
> If I compare the before / after this change, if previously
> pmem_attach_disk() was called in a configuration with FS_DAX=n, it would
> result in a NULL pointer dereference.
I was wrong. drivers/nvdimm/Kconfig has:
config BLK_DEV_PMEM
select DAX
and
drivers/nvdimm/Makefile has:
obj-$(CONFIG_BLK_DEV_PMEM) += nd_pmem.o
nd_pmem-y := pmem.o
which means that anything in pmem.c can assume that alloc_dax() is
implemented.
[...]
>> diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c
>> index 4b7ecd4fd431..f911e58a24dd 100644
>> --- a/drivers/s390/block/dcssblk.c
>> +++ b/drivers/s390/block/dcssblk.c
>> @@ -681,12 +681,14 @@ dcssblk_add_store(struct device *dev, struct
>> device_attribute *attr, const char
>> if (IS_ERR(dev_info->dax_dev)) {
>> rc = PTR_ERR(dev_info->dax_dev);
>> dev_info->dax_dev = NULL;
>> - goto put_dev;
>> + if (rc != -EOPNOTSUPP)
>> + goto put_dev;
>
> config DCSSBLK selects FS_DAX_LIMITED and DAX.
>
> I'm not sure what selecting DAX is trying to achieve here, because the
> Kconfig option is "FS_DAX".
>
> So depending on the real motivation behind this select, we may want to
> consider failure rather than success in the -EOPNOTSUPP case.
>
I missed that alloc_dax() is implemented as not supported based on
CONFIG_DAX (not CONFIG_FS_DAX).
Therefore DCSSBLK Kconfig does the right thing and always selects DAX,
and thus an implemented version of alloc_dax().
This takes care of two of my open questions at least. :)
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
Powered by blists - more mailing lists