From: Christoph Hellwig <hch@lst.de> Date: 2021-09-20 07:28:27
Hi Dan and Jens,
this series fixed a regression in how the dax/write_cache attribute of the
pmem devices was registere. It does so by both fixing the API abuse in the
driver and (temporarily) the behavior change in the block layer that made
this API abuse not work anymore.
Diffstat:
block/genhd.c | 3 +-
drivers/dax/super.c | 64 --------------------------------------------------
drivers/nvdimm/pmem.c | 48 ++++++++++++++++++++++++++++++++++---
include/linux/dax.h | 2 -
4 files changed, 46 insertions(+), 71 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2021-09-20 07:29:07
The recent block layer refactoring broke the way how the pmem driver
abused device_add_disk. Fix this by properly passing the attribute groups
to device_add_disk.
Fixes: 52b85909f85d ("block: fold register_disk into device_add_disk")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/nvdimm/pmem.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2021-09-20 07:29:33
dax_attribute_group is only used by the pmem driver, and can avoid the
completely pointless lookup by the disk name if moved there.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/dax/super.c | 64 -------------------------------------------
drivers/nvdimm/pmem.c | 43 +++++++++++++++++++++++++++++
include/linux/dax.h | 2 --
3 files changed, 43 insertions(+), 66 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2021-09-20 07:29:59
The proper API is to pass the groups to device_add_disk, but the code
used to also allow groups being set before calling *add_disk. Warn
about that but keep the group pointer intact for now so that it can
be removed again after a grace period.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Fixes: 52b85909f85d ("block: fold register_disk into device_add_disk")
---
block/genhd.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
I want to say this should be dax_get_private()... However, looking at the use
of dax_get_private() not a single caller checks for NULL! :-(
So now I wonder why dax_get_private() exists... :-/
A quick history search does not make anything apparent. When the DAXDEV_ALIVE
check was added to dax_get_private() no callers were changed to account for a
potential NULL return.
Dan?
From: Ira Weiny <hidden> Date: 2021-09-20 22:54:23
On Mon, Sep 20, 2021 at 09:27:24AM +0200, Christoph Hellwig wrote:
The recent block layer refactoring broke the way how the pmem driver
abused device_add_disk. Fix this by properly passing the attribute groups
to device_add_disk.
Fixes: 52b85909f85d ("block: fold register_disk into device_add_disk")
Signed-off-by: Christoph Hellwig <hch@lst.de>
From: Ira Weiny <hidden> Date: 2021-09-20 22:54:49
On Mon, Sep 20, 2021 at 09:27:26AM +0200, Christoph Hellwig wrote:
The proper API is to pass the groups to device_add_disk, but the code
used to also allow groups being set before calling *add_disk. Warn
about that but keep the group pointer intact for now so that it can
be removed again after a grace period.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Fixes: 52b85909f85d ("block: fold register_disk into device_add_disk")
From: Dan Williams <hidden> Date: 2021-09-21 02:17:40
On Mon, Sep 20, 2021 at 12:29 AM Christoph Hellwig [off-list ref] wrote:
The recent block layer refactoring broke the way how the pmem driver
abused device_add_disk. Fix this by properly passing the attribute groups
to device_add_disk.
Fixes: 52b85909f85d ("block: fold register_disk into device_add_disk")
This also fixes the the way the pmem driver abused device_add_disk(),
so perhaps add:
Fixes: fef912bf860e ("block: genhd: add 'groups' argument to device_add_disk")
...as well. It's not a stable fix as this is only a cosmetic fixup
until the most recent refactoring turned it into a bug.
Either way, you can add:
Reviewed-by: Dan Williams <redacted>
I want to say this should be dax_get_private()... However, looking at the use
No, this wants to do from @dev to @dax_dev. dax_get_private() assumes
that @dax_dev is already known. Also, in this case @dev is the gendisk
device, so this is a gendisk-to-dax-device with special knowledge that
the gendisk is for a pmem device.
of dax_get_private() not a single caller checks for NULL! :-(
All the callers are correctly assuming that their usage is before kill_dax().
So now I wonder why dax_get_private() exists... :-/
It exists so that the definition of 'struct dax_device' can remain
private, as no one should be directly mucking with dax_device
internals outside of the provided APIs.
A quick history search does not make anything apparent. When the DAXDEV_ALIVE
check was added to dax_get_private() no callers were changed to account for a
potential NULL return.
Dan?
From: Dan Williams <hidden> Date: 2021-09-21 02:17:41
On Mon, Sep 20, 2021 at 12:30 AM Christoph Hellwig [off-list ref] wrote:
quoted hunk
The proper API is to pass the groups to device_add_disk, but the code
used to also allow groups being set before calling *add_disk. Warn
about that but keep the group pointer intact for now so that it can
be removed again after a grace period.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Fixes: 52b85909f85d ("block: fold register_disk into device_add_disk")
---
block/genhd.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
That feels too compact to me, and dev_WARN_ONCE() might save someone a
git blame to look up the reason for the warning:
dev_WARN_ONCE(parent, ddev->groups, "unexpected pre-populated
attribute group\n");
if (!ddev->groups)
ddev->groups = groups;
...but not a deal breaker. Either way you can add:
Reviewed-by: Dan Williams <redacted>
Jens, I'm ok for the final spin of this series to go through block.git
since the referenced commits in Fixes: went that route, just let me
know if you want me to take them.
That feels too compact to me, and dev_WARN_ONCE() might save someone a
git blame to look up the reason for the warning:
dev_WARN_ONCE(parent, ddev->groups, "unexpected pre-populated
attribute group\n");
if (!ddev->groups)
ddev->groups = groups;
...but not a deal breaker. Either way you can add:
I'd rather keep it simple and optmize for the normal case..