Re: [PATCH 1/6] genhd: drop 'bool' argument from __device_add_disk()
On Mon, 2018-07-30 at 10:57 +0200, Hannes Reinecke wrote: > On 07/30/2018 10:56 AM, Christoph Hellwig wrote: > > I really don't see the point for this change. > > Okay, I'll be dropping it on the next iteration. Hello Hannes, Have you already decided when you will post the next iteration of this patch series? Thanks, Bart.
Re: [PATCH 3/6] nvme: register ns_id attributes as default sysfs groups
On Mon, 2018-07-30 at 09:12 +0200, Hannes Reinecke wrote: > @@ -3061,11 +3066,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, > unsigned nsid) > > nvme_get_ctrl(ctrl); > > - device_add_disk(ctrl->device, ns->disk, NULL); > - if (sysfs_create_group(_to_dev(ns->disk)->kobj, > - _ns_id_attr_group)) > - pr_warn("%s: failed to create sysfs group for identification\n", > - ns->disk->disk_name); > + device_add_disk(ctrl->device, ns->disk, nvme_ns_id_attr_groups); > if (ns->ndev && nvme_nvm_register_sysfs(ns)) > pr_warn("%s: failed to register lightnvm sysfs group for > identification\n", > ns->disk->disk_name); Hello Hannes, Have you noticed that nvme_nvm_register_sysfs() also registers sysfs attributes and hence that the attributes registered by that function should be merged into the nvme_ns_id_attr_groups array? Thanks, Bart.
Re: [PATCH v2 1/2] block: Introduce alloc_disk_node_attr()
On Mon, Aug 13, 2018 at 10:25:30AM -0700, Bart Van Assche wrote: > This patch does not change the behavior of any existing code but > introduces a function that will be used by the next patch. "next" is hard to determine in a git log :) Try being a bit more specific as to why you are doing this. thanks, greg k-h
Re: [PATCH v2 0/2] Fix a race condition related to creation of /dev/nvme0n
On Mon, 2018-08-13 at 20:42 +0200, Hannes Reinecke wrote: > I have fixed the very same thing with my patchset titled "genhd: > register default groups with device_add_disk" which is waiting for > inclusion. Thanks Hannes for the feedback. I will drop this patch series and review your patches instead. Bart.
Re: [PATCH v2 0/2] Fix a race condition related to creation of /dev/nvme0n
On 08/13/2018 07:25 PM, Bart Van Assche wrote: Hello Jens, The two patches in this series fix a race condition related to the creation of the sysfs attributes for the /dev/nvme0n devices nodes. I encountered this race while adding tests to the blktests project for the NVMeOF kernel drivers. Please consider these patches for the upstream kernel. Thanks, Bart. Bart Van Assche (2): block: Introduce alloc_disk_node_attr() nvme: Fix race conditions related to creation of /dev/nvme0n block/genhd.c| 26 -- drivers/nvme/host/core.c | 17 + drivers/nvme/host/lightnvm.c | 34 +++--- drivers/nvme/host/nvme.h | 9 - include/linux/genhd.h| 13 +++-- 5 files changed, 51 insertions(+), 48 deletions(-) I have fixed the very same thing with my patchset titled "genhd: register default groups with device_add_disk" which is waiting for inclusion. Cheers, Hannes
[PATCH v2 1/2] block: Introduce alloc_disk_node_attr()
This patch does not change the behavior of any existing code but introduces a function that will be used by the next patch. Signed-off-by: Bart Van Assche Cc: Keith Busch Cc: Christoph Hellwig Cc: Greg Kroah-Hartman Cc: Sagi Grimberg Cc: Matias Bjorling Cc: --- block/genhd.c | 26 -- include/linux/genhd.h | 13 +++-- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/block/genhd.c b/block/genhd.c index 8cc719a37b32..3b643152907d 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -1413,10 +1413,20 @@ dev_t blk_lookup_devt(const char *name, int partno) } EXPORT_SYMBOL(blk_lookup_devt); -struct gendisk *__alloc_disk_node(int minors, int node_id) +/** + * __alloc_disk_node - allocate a gendisk structure and initialize it + * @minors: number of partitions to allocate memory for + * @node_id: NUMA node to allocate memory from + * @ag: NULL or pointer to a NULL-terminated array with attribute groups that + * must be attached to the disk_to_dev() of the allocated disk structure + */ +struct gendisk *__alloc_disk_node(int minors, int node_id, + const struct attribute_group **ag) { struct gendisk *disk; struct disk_part_tbl *ptbl; + const struct attribute_group **agp; + int num_groups = 0; if (minors > DISK_MAX_PARTS) { printk(KERN_ERR @@ -1425,7 +1435,15 @@ struct gendisk *__alloc_disk_node(int minors, int node_id) minors = DISK_MAX_PARTS; } - disk = kzalloc_node(sizeof(struct gendisk), GFP_KERNEL, node_id); + if (ag) { + for (agp = ag; *agp; agp++) + num_groups++; + /* Only count the terminating NULL if the array is not empty. */ + if (num_groups) + num_groups++; + } + disk = kzalloc_node(struct_size(disk, ag, num_groups), GFP_KERNEL, + node_id); if (disk) { if (!init_part_stats(>part0)) { kfree(disk); @@ -1461,6 +1479,10 @@ struct gendisk *__alloc_disk_node(int minors, int node_id) rand_initialize_disk(disk); disk_to_dev(disk)->class = _class; disk_to_dev(disk)->type = _type; + if (num_groups) { + memcpy(disk->ag, ag, num_groups * sizeof(ag)); + disk_to_dev(disk)->groups = disk->ag; + } device_initialize(disk_to_dev(disk)); } return disk; diff --git a/include/linux/genhd.h b/include/linux/genhd.h index 57864422a2c8..50ab6cf77fdf 100644 --- a/include/linux/genhd.h +++ b/include/linux/genhd.h @@ -211,6 +211,11 @@ struct gendisk { int node_id; struct badblocks *bb; struct lockdep_map lockdep_map; + /* +* Storage space for NULL-terminated attribute group array for +* disk_to_dev() of this gendisk structure. +*/ + const struct attribute_group *ag[0]; }; static inline struct gendisk *part_to_disk(struct hd_struct *part) @@ -608,7 +613,8 @@ extern void __delete_partition(struct percpu_ref *); extern void delete_partition(struct gendisk *, int); extern void printk_all_partitions(void); -extern struct gendisk *__alloc_disk_node(int minors, int node_id); +extern struct gendisk *__alloc_disk_node(int minors, int node_id, +const struct attribute_group **ag); extern struct kobject *get_disk_and_module(struct gendisk *disk); extern void put_disk(struct gendisk *disk); extern void put_disk_and_module(struct gendisk *disk); @@ -634,6 +640,9 @@ extern ssize_t part_fail_store(struct device *dev, #endif /* CONFIG_FAIL_MAKE_REQUEST */ #define alloc_disk_node(minors, node_id) \ + alloc_disk_node_attr(minors, node_id, NULL) + +#define alloc_disk_node_attr(minors, node_id, ag) \ ({ \ static struct lock_class_key __key; \ const char *__name; \ @@ -641,7 +650,7 @@ extern ssize_t part_fail_store(struct device *dev, \ __name = "(gendisk_completion)"#minors"("#node_id")"; \ \ - __disk = __alloc_disk_node(minors, node_id);\ + __disk = __alloc_disk_node(minors, node_id, ag);\ \ if (__disk) \ lockdep_init_map(&__disk->lockdep_map, __name, &__key, 0); \ -- 2.18.0
[PATCH v2 2/2] nvme: Fix race conditions related to creation of /dev/nvme0n
For the udev rules that create symbolic links under /dev it is essential that all sysfs attributes are registered before an object becomes visible. This patch avoids that udevd fails to create the /dev/disk/by-id/nvme-uuid.* symbolic link. Fixes: 2b9b6e86bca7 ("NVMe: Export namespace attributes to sysfs") Signed-off-by: Bart Van Assche Cc: Keith Busch Cc: Christoph Hellwig Cc: Greg Kroah-Hartman Cc: Sagi Grimberg Cc: Matias Bjorling Cc: --- drivers/nvme/host/core.c | 17 + drivers/nvme/host/lightnvm.c | 34 +++--- drivers/nvme/host/nvme.h | 9 - 3 files changed, 16 insertions(+), 44 deletions(-) diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index dd8ec1dd9219..0121db66d2c6 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -3043,6 +3043,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) struct nvme_id_ns *id; char disk_name[DISK_NAME_LEN]; int node = dev_to_node(ctrl->dev), flags = GENHD_FL_EXT_DEVT; + const struct attribute_group *nvme_attr_groups[3]; ns = kzalloc_node(sizeof(*ns), GFP_KERNEL, node); if (!ns) @@ -3080,7 +3081,10 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) } } - disk = alloc_disk_node(0, node); + nvme_attr_groups[0] = _ns_id_attr_group; + nvme_attr_groups[1] = nvme_nvm_attr_group(ns); + nvme_attr_groups[2] = NULL; + disk = alloc_disk_node_attr(0, node, nvme_attr_groups); if (!disk) goto out_unlink_ns; @@ -3100,13 +3104,6 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) nvme_get_ctrl(ctrl); device_add_disk(ctrl->device, ns->disk); - if (sysfs_create_group(_to_dev(ns->disk)->kobj, - _ns_id_attr_group)) - pr_warn("%s: failed to create sysfs group for identification\n", - ns->disk->disk_name); - if (ns->ndev && nvme_nvm_register_sysfs(ns)) - pr_warn("%s: failed to register lightnvm sysfs group for identification\n", - ns->disk->disk_name); nvme_mpath_add_disk(ns, id); nvme_fault_inject_init(ns); @@ -3132,10 +3129,6 @@ static void nvme_ns_remove(struct nvme_ns *ns) nvme_fault_inject_fini(ns); if (ns->disk && ns->disk->flags & GENHD_FL_UP) { - sysfs_remove_group(_to_dev(ns->disk)->kobj, - _ns_id_attr_group); - if (ns->ndev) - nvme_nvm_unregister_sysfs(ns); del_gendisk(ns->disk); blk_cleanup_queue(ns->queue); if (blk_get_integrity(ns->disk)) diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c index 6fe5923c95d4..11e687f3c36f 100644 --- a/drivers/nvme/host/lightnvm.c +++ b/drivers/nvme/host/lightnvm.c @@ -1270,39 +1270,19 @@ static const struct attribute_group nvm_dev_attr_group_20 = { .attrs = nvm_dev_attrs_20, }; -int nvme_nvm_register_sysfs(struct nvme_ns *ns) +const struct attribute_group *nvme_nvm_attr_group(const struct nvme_ns *ns) { - struct nvm_dev *ndev = ns->ndev; - struct nvm_geo *geo = >geo; + const struct nvm_dev *ndev = ns->ndev; if (!ndev) - return -EINVAL; + return NULL; - switch (geo->major_ver_id) { + switch (ndev->geo->major_ver_id) { case 1: - return sysfs_create_group(_to_dev(ns->disk)->kobj, - _dev_attr_group_12); + return _dev_attr_group_12; case 2: - return sysfs_create_group(_to_dev(ns->disk)->kobj, - _dev_attr_group_20); + return _dev_attr_group_20; } - return -EINVAL; -} - -void nvme_nvm_unregister_sysfs(struct nvme_ns *ns) -{ - struct nvm_dev *ndev = ns->ndev; - struct nvm_geo *geo = >geo; - - switch (geo->major_ver_id) { - case 1: - sysfs_remove_group(_to_dev(ns->disk)->kobj, - _dev_attr_group_12); - break; - case 2: - sysfs_remove_group(_to_dev(ns->disk)->kobj, - _dev_attr_group_20); - break; - } + return NULL; } diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index bb4a2003c097..112596ad1ed3 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -551,8 +551,7 @@ static inline void nvme_mpath_stop(struct nvme_ctrl *ctrl) void nvme_nvm_update_nvm_info(struct nvme_ns *ns); int nvme_nvm_register(struct nvme_ns *ns, char *disk_name, int node); void nvme_nvm_unregister(struct nvme_ns *ns); -int nvme_nvm_register_sysfs(struct nvme_ns *ns); -void nvme_nvm_unregister_sysfs(struct nvme_ns
[PATCH v2 0/2] Fix a race condition related to creation of /dev/nvme0n
Hello Jens, The two patches in this series fix a race condition related to the creation of the sysfs attributes for the /dev/nvme0n devices nodes. I encountered this race while adding tests to the blktests project for the NVMeOF kernel drivers. Please consider these patches for the upstream kernel. Thanks, Bart. Bart Van Assche (2): block: Introduce alloc_disk_node_attr() nvme: Fix race conditions related to creation of /dev/nvme0n block/genhd.c| 26 -- drivers/nvme/host/core.c | 17 + drivers/nvme/host/lightnvm.c | 34 +++--- drivers/nvme/host/nvme.h | 9 - include/linux/genhd.h| 13 +++-- 5 files changed, 51 insertions(+), 48 deletions(-) -- 2.18.0
Re: [PATCH v6 08/12] block, scsi: Introduce blk_pm_runtime_exit()
On Mon, 2018-08-13 at 17:24 +0800, jianchao.wang wrote: > I'm afraid this will not work. Since this patch fixes a bug that nobody has reported so far and since no later patches rely on this patch, I will leave it out. Thanks, Bart.
Re: [PATCH v6 08/12] block, scsi: Introduce blk_pm_runtime_exit()
Hi Bart On 08/11/2018 12:17 AM, Bart Van Assche wrote: > void blk_pm_runtime_exit(struct request_queue *q) > { > if (!q->dev) > return; > > pm_runtime_get_sync(q->dev); > blk_freeze_queue(q); > q->dev = NULL; > blk_unfreeze_queue(q); > } I'm afraid this will not work. In following patch: @@ -972,6 +972,8 @@ int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags) if (success) return 0; + blk_pm_request_resume(q); + if (flags & BLK_MQ_REQ_NOWAIT) return -EBUSY; We could still invoke blk_pm_request_resume even if the queue is frozen. And we have blk_pm_request_resume later as following: +static inline void blk_pm_request_resume(struct request_queue *q) +{ + if (q->dev && (q->rpm_status == RPM_SUSPENDED || + q->rpm_status == RPM_SUSPENDING)) + pm_request_resume(q->dev); +} Thanks Jianchao