* [PATCH 01/25] bdi: Provide bdi_register_va() and bdi_alloc()
From: Jan Kara @ 2017-03-29 10:55 UTC (permalink / raw)
To: linux-fsdevel; +Cc: linux-block, Christoph Hellwig, Jan Kara
In-Reply-To: <20170329105623.18241-1-jack@suse.cz>
Add function that registers bdi and takes va_list instead of variable
number of arguments.
Add bdi_alloc() as simple wrapper for NUMA-unaware users allocating BDI.
Signed-off-by: Jan Kara <jack@suse.cz>
---
include/linux/backing-dev.h | 6 ++++++
mm/backing-dev.c | 20 +++++++++++++++-----
2 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index c52a48cb9a66..47a98e6e2a65 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -30,6 +30,8 @@ void bdi_put(struct backing_dev_info *bdi);
__printf(3, 4)
int bdi_register(struct backing_dev_info *bdi, struct device *parent,
const char *fmt, ...);
+int bdi_register_va(struct backing_dev_info *bdi, struct device *parent,
+ const char *fmt, va_list args);
int bdi_register_dev(struct backing_dev_info *bdi, dev_t dev);
int bdi_register_owner(struct backing_dev_info *bdi, struct device *owner);
void bdi_unregister(struct backing_dev_info *bdi);
@@ -37,6 +39,10 @@ void bdi_unregister(struct backing_dev_info *bdi);
int __must_check bdi_setup_and_register(struct backing_dev_info *, char *);
void bdi_destroy(struct backing_dev_info *bdi);
struct backing_dev_info *bdi_alloc_node(gfp_t gfp_mask, int node_id);
+static inline struct backing_dev_info *bdi_alloc(gfp_t gfp_mask)
+{
+ return bdi_alloc_node(gfp_mask, NUMA_NO_NODE);
+}
void wb_start_writeback(struct bdi_writeback *wb, long nr_pages,
bool range_cyclic, enum wb_reason reason);
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 3ea3bbd921d6..e5e0972bdd6f 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -856,18 +856,15 @@ struct backing_dev_info *bdi_alloc_node(gfp_t gfp_mask, int node_id)
return bdi;
}
-int bdi_register(struct backing_dev_info *bdi, struct device *parent,
- const char *fmt, ...)
+int bdi_register_va(struct backing_dev_info *bdi, struct device *parent,
+ const char *fmt, va_list args)
{
- va_list args;
struct device *dev;
if (bdi->dev) /* The driver needs to use separate queues per device */
return 0;
- va_start(args, fmt);
dev = device_create_vargs(bdi_class, parent, MKDEV(0, 0), bdi, fmt, args);
- va_end(args);
if (IS_ERR(dev))
return PTR_ERR(dev);
@@ -884,6 +881,19 @@ int bdi_register(struct backing_dev_info *bdi, struct device *parent,
trace_writeback_bdi_register(bdi);
return 0;
}
+EXPORT_SYMBOL(bdi_register_va);
+
+int bdi_register(struct backing_dev_info *bdi, struct device *parent,
+ const char *fmt, ...)
+{
+ va_list args;
+ int ret;
+
+ va_start(args, fmt);
+ ret = bdi_register_va(bdi, parent, fmt, args);
+ va_end(args);
+ return ret;
+}
EXPORT_SYMBOL(bdi_register);
int bdi_register_dev(struct backing_dev_info *bdi, dev_t dev)
--
2.10.2
^ permalink raw reply related
* [PATCH 0/25 v2] fs: Convert all embedded bdis into separate ones
From: Jan Kara @ 2017-03-29 10:55 UTC (permalink / raw)
To: linux-fsdevel
Cc: linux-block, Christoph Hellwig, Jan Kara, linux-mtd, linux-nfs,
Petr Vandrovec, linux-nilfs, cluster-devel, osd-dev, codalist,
linux-afs, ecryptfs, linux-cifs, ceph-devel, linux-btrfs,
v9fs-developer, lustre-devel
Hello,
this is the second revision of the patch series which converts all embedded
occurences of struct backing_dev_info to use standalone dynamically allocated
structures. This makes bdi handling unified across all bdi users and generally
removes some boilerplate code from filesystems setting up their own bdi. It
also allows us to remove some code from generic bdi implementation.
The patches were only compile-tested for most filesystems (I've tested
mounting only for NFS & btrfs) so fs maintainers please have a look whether
the changes look sound to you.
This series is based on top of bdi fixes that were merged into linux-block
git tree into for-next branch. I have pushed out the result as a branch to
git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git bdi
Changes since v1:
* Added some acks
* Added further FUSE cleanup patch
* Added removal of unused argument to bdi_register()
* Fixed up some compilation failures spotted by 0-day testing
Honza
^ permalink raw reply
* Outstanding MQ questions from MMC
From: Linus Walleij @ 2017-03-29 3:09 UTC (permalink / raw)
To: linux-mmc@vger.kernel.org, linux-block, Jens Axboe,
Christoph Hellwig
Cc: Ulf Hansson, Adrian Hunter, Paolo Valente
Hi folks,
I earlier left some unanswered questions in my MMC to MQ conversion series
but I figured it is better if I collect them and ask the blk-mq
maintainers directly
how to deal with the following situations that occur in the MMC block layer:
1. The current MMC code locks the host when the first request comes in
from blk_fetch_request() and unlocks it when blk_fetch_request() returns
NULL twice in a row. Then the polling thread terminated and is not restarted
until we get called by the mmc_request_fn.
Host locking means that we will not send other commands to the MMC
card from i.e. userspace, which sometimes can send spurious stuff orthogonal
to the block layer. If the block layer has locked the host, userspace
has to wait
and vice versa. It is not a common contention point but it still happens.
In MQ, I have simply locked the host on the first request and then I never
release it. Clearly this does not work. I am uncertain on how to handle this
and whether MQ has a way to tell us that the queue is empty so we may release
the host. I toyed with the idea to just set up a timer, but a "queue
empty" callback
from the block layer is what would be ideal.
2. When MMC cards are ejected a serious error condition occurs. So for this
reason we spool out the queue with
req->rq_flags |= RQF_QUIET;
blk_end_request_all(req, -EIO);
This will shut up a huge amount of console errors for example.
I have no clue on how to manage this with MQ. I am currently using
blk_mq_complete_request(mq_rq->req, -EIO);
and nothing else, and it will hit all requests for the ejected card coming
in from this point. Is this the right solution? Or is there some medium
eject handling I'm not aware of inside the MQ layer? It seems like something
that can happen on pluggable harddrives and CDROMS and what not.
3. Sometimes a read or write gets partially completed. Say we read 12 out
of 15 sectors or somthing like that. I have no idea how often this occurs in
practice. With the old block layer we did this:
blk_end_request(req, 0, bytes_xfered);
It seems MQ cannot handle partially completed transfers. I currently do this:
blk_mq_requeue_request(req, false);
I do not feel that is the right thing to do either, and would like
recommendations
on how to proceed with this.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v3 0/4] block: misc changes
From: Ming Lei @ 2017-03-29 2:11 UTC (permalink / raw)
To: Jens Axboe, linux-block, Christoph Hellwig
Cc: Bart Van Assche, Hannes Reinecke
In-Reply-To: <20170327120658.29864-1-tom.leiming@gmail.com>
On Mon, Mar 27, 2017 at 08:06:54PM +0800, Ming Lei wrote:
> Hi,
>
> The 1st patch add comments on blk-mq races with timeout handler.
>
> The other 3 patches improves handling for dying queue:
> - the 2nd one adds one barrier in blk_queue_enter() for
> avoiding hanging caused by out-of-order
> - the 3rd and 4th patches block new I/O entering queue
> after queue is set as dying
>
> V3:
> - tweak comments as suggested by Bart Van Assche
Hi Jens,
This patchset has been posted several times without real code change
for a while, could you consider it for v4.12?
Thanks,
Ming
^ permalink raw reply
* Re: [PATCH] block-mq: don't re-queue if we get a queue error
From: Ming Lei @ 2017-03-29 2:04 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-block, kernel-team
In-Reply-To: <1490733472-3088-1-git-send-email-jbacik@fb.com>
On Tue, Mar 28, 2017 at 04:37:52PM -0400, Josef Bacik wrote:
> When try to issue a request directly and we fail we will requeue the
> request, but call blk_mq_end_request() as well. This leads to the
> completed request being on a queuelist and getting ended twice, which
> causes list corruption in schedulers and other shenanigans.
>
> Signed-off-by: Josef Bacik <jbacik@fb.com>
> ---
> block/blk-mq.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 08a49c6..c5a6985 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1457,8 +1457,6 @@ static void blk_mq_try_issue_directly(struct request *rq, blk_qc_t *cookie,
> return;
> }
>
> - __blk_mq_requeue_request(rq);
> -
> if (ret == BLK_MQ_RQ_QUEUE_ERROR) {
> *cookie = BLK_QC_T_NONE;
> rq->errors = -EIO;
> @@ -1466,6 +1464,7 @@ static void blk_mq_try_issue_directly(struct request *rq, blk_qc_t *cookie,
> return;
> }
>
> + __blk_mq_requeue_request(rq);
> insert:
> blk_mq_sched_insert_request(rq, false, true, false, may_sleep);
> }
Reviewed-by: Ming Lei <tom.leiming@gmail.com>
--
Ming
^ permalink raw reply
* [PATCH v2 2/3] blk-mq: fix leak of q->stats
From: Omar Sandoval @ 2017-03-28 23:12 UTC (permalink / raw)
To: Jens Axboe, linux-block; +Cc: kernel-team
In-Reply-To: <91d938665e5386f10f8756ee8c7ef3e6496ea260.1490742717.git.osandov@fb.com>
From: Omar Sandoval <osandov@fb.com>
blk_alloc_queue_node() already allocates q->stats, so
blk_mq_init_allocated_queue() is overwriting it with a new allocation.
Fixes: a83b576c9c25 ("block: fix stacked driver stats init and free")
Reviewed-by: Ming Lei <tom.leiming@gmail.com>
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
block/blk-mq.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 7df9dbfab022..00eeef31db30 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2212,10 +2212,6 @@ struct request_queue *blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
/* mark the queue as mq asap */
q->mq_ops = set->ops;
- q->stats = blk_alloc_queue_stats();
- if (!q->stats)
- goto err_exit;
-
q->poll_cb = blk_stat_alloc_callback(blk_mq_poll_stats_fn,
blk_stat_rq_ddir, 2, q);
if (!q->poll_cb)
--
2.12.1
^ permalink raw reply related
* [PATCH v2 1/3] block: warn if sharing request queue across gendisks
From: Omar Sandoval @ 2017-03-28 23:12 UTC (permalink / raw)
To: Jens Axboe, linux-block; +Cc: kernel-team
From: Omar Sandoval <osandov@fb.com>
Now that the remaining drivers have been converted to one request queue
per gendisk, let's warn if a request queue gets registered more than
once. This will catch future drivers which might do it inadvertently or
any old drivers that I may have missed.
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
block/blk-sysfs.c | 7 +++++++
include/linux/blkdev.h | 1 +
2 files changed, 8 insertions(+)
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 7f090dd15ca6..833fb7f9ce9d 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -871,6 +871,11 @@ int blk_register_queue(struct gendisk *disk)
if (WARN_ON(!q))
return -ENXIO;
+ WARN_ONCE(test_bit(QUEUE_FLAG_REGISTERED, &q->queue_flags),
+ "%s is registering an already registered queue\n",
+ kobject_name(&dev->kobj));
+ queue_flag_set_unlocked(QUEUE_FLAG_REGISTERED, q);
+
/*
* SCSI probing may synchronously create and destroy a lot of
* request_queues for non-existent devices. Shutting down a fully
@@ -931,6 +936,8 @@ void blk_unregister_queue(struct gendisk *disk)
if (WARN_ON(!q))
return;
+ queue_flag_clear_unlocked(QUEUE_FLAG_REGISTERED, q);
+
if (q->mq_ops)
blk_mq_unregister_dev(disk_to_dev(disk), q);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 1a7dc42a8918..a2dc6b390d48 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -617,6 +617,7 @@ struct request_queue {
#define QUEUE_FLAG_STATS 27 /* track rq completion times */
#define QUEUE_FLAG_RESTART 28 /* queue needs restart at completion */
#define QUEUE_FLAG_POLL_STATS 29 /* collecting stats for hybrid polling */
+#define QUEUE_FLAG_REGISTERED 30 /* queue has been registered to a disk */
#define QUEUE_FLAG_DEFAULT ((1 << QUEUE_FLAG_IO_STAT) | \
(1 << QUEUE_FLAG_STACKABLE) | \
--
2.12.1
^ permalink raw reply related
* [PATCH v2 3/3] block: fix leak of q->rq_wb
From: Omar Sandoval @ 2017-03-28 23:12 UTC (permalink / raw)
To: Jens Axboe, linux-block; +Cc: kernel-team
In-Reply-To: <91d938665e5386f10f8756ee8c7ef3e6496ea260.1490742717.git.osandov@fb.com>
From: Omar Sandoval <osandov@fb.com>
CONFIG_DEBUG_TEST_DRIVER_REMOVE found a possible leak of q->rq_wb when a
request queue is reregistered. This has been a problem since wbt was
introduced, but the WARN_ON(!list_empty(&stats->callbacks)) in the
blk-stat rework exposed it. Fix it by cleaning up wbt when we unregister
the queue.
Fixes: 87760e5eef35 ("block: hook up writeback throttling")
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
block/blk-sysfs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 833fb7f9ce9d..45854266e398 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -795,7 +795,6 @@ static void blk_release_queue(struct kobject *kobj)
struct request_queue *q =
container_of(kobj, struct request_queue, kobj);
- wbt_exit(q);
if (test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags))
blk_stat_remove_callback(q, q->poll_cb);
blk_stat_free_callback(q->poll_cb);
@@ -938,6 +937,9 @@ void blk_unregister_queue(struct gendisk *disk)
queue_flag_clear_unlocked(QUEUE_FLAG_REGISTERED, q);
+ wbt_exit(q);
+
+
if (q->mq_ops)
blk_mq_unregister_dev(disk_to_dev(disk), q);
--
2.12.1
^ permalink raw reply related
* Re: [PATCH v5] blkcg: allocate struct blkcg_gq outside request queue spinlock
From: Tahsin Erdogan @ 2017-03-28 22:01 UTC (permalink / raw)
To: Jens Axboe; +Cc: Tejun Heo, linux-block, David Rientjes, linux-kernel
In-Reply-To: <835b6175-6f18-164c-2363-74023ae6cbcb@kernel.dk>
Thanks!
On Tue, Mar 28, 2017 at 2:59 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 03/09/2017 01:05 AM, Tahsin Erdogan wrote:
>> blkg_conf_prep() currently calls blkg_lookup_create() while holding
>> request queue spinlock. This means allocating memory for struct
>> blkcg_gq has to be made non-blocking. This causes occasional -ENOMEM
>> failures in call paths like below:
>>
>> pcpu_alloc+0x68f/0x710
>> __alloc_percpu_gfp+0xd/0x10
>> __percpu_counter_init+0x55/0xc0
>> cfq_pd_alloc+0x3b2/0x4e0
>> blkg_alloc+0x187/0x230
>> blkg_create+0x489/0x670
>> blkg_lookup_create+0x9a/0x230
>> blkg_conf_prep+0x1fb/0x240
>> __cfqg_set_weight_device.isra.105+0x5c/0x180
>> cfq_set_weight_on_dfl+0x69/0xc0
>> cgroup_file_write+0x39/0x1c0
>> kernfs_fop_write+0x13f/0x1d0
>> __vfs_write+0x23/0x120
>> vfs_write+0xc2/0x1f0
>> SyS_write+0x44/0xb0
>> entry_SYSCALL_64_fastpath+0x18/0xad
>>
>> In the code path above, percpu allocator cannot call vmalloc() due to
>> queue spinlock.
>>
>> A failure in this call path gives grief to tools which are trying to
>> configure io weights. We see occasional failures happen shortly after
>> reboots even when system is not under any memory pressure. Machines
>> with a lot of cpus are more vulnerable to this condition.
>>
>> Update blkg_create() function to temporarily drop the rcu and queue
>> locks when it is allowed by gfp mask.
>
> Added for 4.12, thanks for your persistence in pulling this through.
>
> --
> Jens Axboe
>
^ permalink raw reply
* Re: [PATCH v5] blkcg: allocate struct blkcg_gq outside request queue spinlock
From: Jens Axboe @ 2017-03-28 21:59 UTC (permalink / raw)
To: Tahsin Erdogan, Tejun Heo; +Cc: linux-block, David Rientjes, linux-kernel
In-Reply-To: <20170309080531.9048-1-tahsin@google.com>
On 03/09/2017 01:05 AM, Tahsin Erdogan wrote:
> blkg_conf_prep() currently calls blkg_lookup_create() while holding
> request queue spinlock. This means allocating memory for struct
> blkcg_gq has to be made non-blocking. This causes occasional -ENOMEM
> failures in call paths like below:
>
> pcpu_alloc+0x68f/0x710
> __alloc_percpu_gfp+0xd/0x10
> __percpu_counter_init+0x55/0xc0
> cfq_pd_alloc+0x3b2/0x4e0
> blkg_alloc+0x187/0x230
> blkg_create+0x489/0x670
> blkg_lookup_create+0x9a/0x230
> blkg_conf_prep+0x1fb/0x240
> __cfqg_set_weight_device.isra.105+0x5c/0x180
> cfq_set_weight_on_dfl+0x69/0xc0
> cgroup_file_write+0x39/0x1c0
> kernfs_fop_write+0x13f/0x1d0
> __vfs_write+0x23/0x120
> vfs_write+0xc2/0x1f0
> SyS_write+0x44/0xb0
> entry_SYSCALL_64_fastpath+0x18/0xad
>
> In the code path above, percpu allocator cannot call vmalloc() due to
> queue spinlock.
>
> A failure in this call path gives grief to tools which are trying to
> configure io weights. We see occasional failures happen shortly after
> reboots even when system is not under any memory pressure. Machines
> with a lot of cpus are more vulnerable to this condition.
>
> Update blkg_create() function to temporarily drop the rcu and queue
> locks when it is allowed by gfp mask.
Added for 4.12, thanks for your persistence in pulling this through.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH] blkcg: allocate struct blkcg_gq outside request queue spinlock
From: Tejun Heo @ 2017-03-28 21:53 UTC (permalink / raw)
To: Jens Axboe; +Cc: Tahsin Erdogan, linux-block, David Rientjes, linux-kernel
In-Reply-To: <5b9ab59c-4121-d49a-1dc5-bd419f3ac94f@kernel.dk>
On Fri, Mar 24, 2017 at 04:04:32PM -0600, Jens Axboe wrote:
> On 03/24/2017 03:56 PM, Tahsin Erdogan wrote:
> > blkg_conf_prep() currently calls blkg_lookup_create() while holding
> > request queue spinlock. This means allocating memory for struct
> > blkcg_gq has to be made non-blocking. This causes occasional -ENOMEM
> > failures in call paths like below:
> >
> > pcpu_alloc+0x68f/0x710
> > __alloc_percpu_gfp+0xd/0x10
> > __percpu_counter_init+0x55/0xc0
> > cfq_pd_alloc+0x3b2/0x4e0
> > blkg_alloc+0x187/0x230
> > blkg_create+0x489/0x670
> > blkg_lookup_create+0x9a/0x230
> > blkg_conf_prep+0x1fb/0x240
> > __cfqg_set_weight_device.isra.105+0x5c/0x180
> > cfq_set_weight_on_dfl+0x69/0xc0
> > cgroup_file_write+0x39/0x1c0
> > kernfs_fop_write+0x13f/0x1d0
> > __vfs_write+0x23/0x120
> > vfs_write+0xc2/0x1f0
> > SyS_write+0x44/0xb0
> > entry_SYSCALL_64_fastpath+0x18/0xad
> >
> > In the code path above, percpu allocator cannot call vmalloc() due to
> > queue spinlock.
> >
> > A failure in this call path gives grief to tools which are trying to
> > configure io weights. We see occasional failures happen shortly after
> > reboots even when system is not under any memory pressure. Machines
> > with a lot of cpus are more vulnerable to this condition.
> >
> > Do struct blkcg_gq allocations outside the queue spinlock to allow
> > blocking during memory allocations.
>
> This looks much simpler/cleaner to me, compared to v5. Tejun, what do
> you think?
So, this patch in itself looks better but now we end up with two
separate mechanisms to handle non-atomic allocations. This drop lock
/ alloc / relock / check invariants in the main path and preallocation
logic used in the init path. Right now, both proposed implementations
aren't that satisfactory. Personally, I'd prefer superficial ugliness
to structural duplications, but, ideally, we shouldn't have to make
this choice. idk, it's a bug fix. We can always clean things up
later.
Acked-by: Tejun Heo <tj@kernel.org>
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH 2/2] blk-stat: add a poll_size value to the request_queue struct
From: Stephen Bates @ 2017-03-28 21:52 UTC (permalink / raw)
To: Jens Axboe, Sagi Grimberg
Cc: linux-block@vger.kernel.org, Damien.LeMoal@wdc.com,
osandov@osandov.com, linux-nvme@lists.infradead.org
In-Reply-To: <fd49a447-153e-cad9-63ca-216ab6db7fb8@kernel.dk>
W1JldHJ5aW5nIGFzIG15IG5ldyBzZXR1cCBzZWNyZXRseSBjb252ZXJ0ZWQgdG8gaHRtbCBmb3Jt
YXQgd2l0aG91dCB0ZWxsaW5nIG1lLiBBcG9sb2dpZXMgZm9yIHRoZSByZXNlbmQuXQ0KDQo+Pj4g
DQo+Pj4gVGhhbmtzIGZvciB0aGUgcmV2aWV3IFNhZ2kuIEnigJlkIGJlIE9LIGdvaW5nIHdpdGgg
PD0wIGFzIHRoZSBleGFjdA0KPj4+IG1hdGNoIHdvdWxkIG5vcm1hbGx5IGJlIGZvciBtaW5pbWFs
IElPIHNpemVzICh3aGVyZSA8PSBhbmQgPSBhcmUgdGhlDQo+Pj4gc2FtZSB0aGluZykuIEkgd2ls
bCBzZWUgd2hhdCBvdGhlciBmZWVkYmFjayBJIGdldCBhbmQgYWltIHRvIGRvIGENCj4+PiByZXNw
aW4gc29vbuKApg0KPj4gDQo+PiBObyB0dW5hYmxlcyBmb3IgdGhpcywgcGxlYXNlLiBUaGVyZSdz
IGFic29sdXRlbHkgbm8gcmVhc29uIHdoeSB3ZSBzaG91bGQNCj4+IG5lZWQgaXQuDQo+DQo+IEpl
bnMg4oCTIGJ5IHRoaXMgeW91IG1lYW4geW91IHdhbnQgdG8gb25seSBidWNrZXQgSU8gdGhhdCBh
cmUgZXhhY3RseQ0KPiB0aGUgbWluaW11bSBibG9jayBzaXplIHN1cHBvcnRlZCBieSB0aGUgdW5k
ZXJseWluZyBibG9jayBkZXZpY2U/IEkgd2FzDQo+IGVudmlzaW9uaW5nIHdlIG1pZ2h0IHdhbnQg
dG8gcmVsYXggdGhhdCBpbiBjZXJ0YWluIGNhc2VzIChlLmcuIGJ1Y2tldA0KPiA0S0IgYW5kIGJl
bG93IGdvaW5nIHRvIGEgNTEyQiBkZXZpY2UpLg0KIA0KPiBTb3JyeSwgdGhlIGFib3ZlIHdhcyBh
IGJpdCB0ZXJzZS4gSSB0aGluayBhIG11Y2ggYmV0dGVyIHNvbHV0aW9uIHdvdWxkDQo+IGJlIHRv
IGNyZWF0ZSBhIG51bWJlciBvZiBidWNrZXRzIChwZXIgZGF0YSBkaXJlY3Rpb24pIGFuZCBkbyBz
dGF0cyBvbg0KPiBhbGwgb2YgdGhlbS4gVGhlIGJ1Y2tldHMgd291bGQgY292ZXIgYSByZWFzb25h
YmxlIHJhbmdlIG9mIHJlcXVlc3QNCj4gc2l6ZXMuIFRoZW4gd2hlbiB5b3UgcG9sbCBmb3IgYSBn
aXZlbiByZXF1ZXN0LCB3ZSBjYW4gYmFzZSBvdXIgdGltZWQNCj4gbnVtYmVyIG9uIHRoZSBkYXRh
IGRpcmVjdGlvbiBBTkQgc2l6ZSBvZiBpdC4gWW91IGNhbiBnZXQgcHJldHR5IGZhciB3aXRoDQo+
IGEgZmV3IGJ1Y2tldHM6DQo+IA0KPiA1MTJiDQo+IDRrDQo+IDhrDQo+IDE2aw0KPiAzMmsNCj4g
NjRrDQo+IDEyOGsNCj4gDQo+IGFuZCB5b3UgY291bGQgZXZlbiBoYXZlIHlvdXIgdGltZSBlc3Rp
bWF0aW9uIGZ1bmN0aW9uIHR1cm4gdGhlc2UgaW50bw0KPiBzb21ldGhpbmcgc2FuZS4gT3IganVz
dCB1c2UgYSBjb21wb3NpdGUgb2YgYnVja2V0cywgaWYgeW91IHdpc2guDQogDQpJIGRpZCBnbyBk
b3duIHRoaXMgcGF0aCBpbml0aWFsbHkgYnV0IHRoZW4gbW92ZWQgYXdheSBmcm9tIGl0IHNpbmNl
IHdlIHdlcmUgZm9jdXNpbmcgb25seSBvbiB0aGUgc21hbGxlciBJTyBzaXplLiBIb3dldmVyIEkg
Y2FuIGRlZmluaXRlbHkgdGFrZSBhIGxvb2sgYXQgdGhpcyBhZ2FpbiBhcyBJIGFncmVlIHRoYXQg
aXQgY291bGQgYmUgbW9yZSB1c2VmdWwgaW4gdGhlIGxvbmcgcnVuLg0KIA0KSSB3b3VsZCBsaWtl
IHRvIGtlZXAgbXkgZmlyc3QgcGF0Y2ggaW4gdGhpcyBzZXJpZXMgYWxpdmUgc2luY2UgSSBkbyB0
aGluayBoYXZpbmcgdGhlIG9wdGlvbiB0byBub3QgYnVja2V0IGFuIElPIGlzIGEgdXNlZnVsIHRo
aW5nIHRvIGhhdmUuDQogDQpJ4oCZbGwgdGFrZSBhbGwgdGhlIGZlZWRiYWNrIHRvIGRhdGUgYW5k
IHdvcmsgb24gYSB2Mi4gVGhhbmtzIQ0KIA0KU3RlcGhlbg0KDQoNCg0K
^ permalink raw reply
* Re: [PATCH 2/2] blk-stat: add a poll_size value to the request_queue struct
From: Stephen Bates @ 2017-03-28 21:49 UTC (permalink / raw)
To: Jens Axboe, Sagi Grimberg
Cc: linux-block@vger.kernel.org, Damien.LeMoal@wdc.com,
osandov@osandov.com, linux-nvme@lists.infradead.org
In-Reply-To: <fd49a447-153e-cad9-63ca-216ab6db7fb8@kernel.dk>
[-- Attachment #1: Type: text/plain, Size: 1711 bytes --]
>>>
>>> Thanks for the review Sagi. I’d be OK going with <=0 as the exact
>>> match would normally be for minimal IO sizes (where <= and = are the
>>> same thing). I will see what other feedback I get and aim to do a
>>> respin soon…
>>
>> No tunables for this, please. There's absolutely no reason why we should
>> need it.
>
> Jens – by this you mean you want to only bucket IO that are exactly
> the minimum block size supported by the underlying block device? I was
> envisioning we might want to relax that in certain cases (e.g. bucket
> 4KB and below going to a 512B device).
> Sorry, the above was a bit terse. I think a much better solution would
> be to create a number of buckets (per data direction) and do stats on
> all of them. The buckets would cover a reasonable range of request
> sizes. Then when you poll for a given request, we can base our timed
> number on the data direction AND size of it. You can get pretty far with
> a few buckets:
>
> 512b
> 4k
> 8k
> 16k
> 32k
> 64k
> 128k
>
> and you could even have your time estimation function turn these into
> something sane. Or just use a composite of buckets, if you wish.
I did go down this path initially but then moved away from it since we were focusing only on the smaller IO size. However I can definitely take a look at this again as I agree that it could be more useful in the long run.
I would like to keep my first patch in this series alive since I do think having the option to not bucket an IO is a useful thing to have.
I’ll take all the feedback to date and work on a v2. Thanks!
Stephen
[-- Attachment #2: Type: text/html, Size: 5137 bytes --]
^ permalink raw reply
* Re: [PATCH 0/6] block: convert remaining drivers which share a request queue
From: Jens Axboe @ 2017-03-28 21:07 UTC (permalink / raw)
To: Omar Sandoval; +Cc: Vivek Goyal, linux-block, kernel-team
In-Reply-To: <cover.1490681910.git.osandov@fb.com>
On Mon, Mar 27 2017, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
>
> This is clearly the pinnacle of my career: converting all remaining
> block drivers which share a request queue across gendisks to use a
> separate request queue per gendisk. These are all compile tested (but
> the last two platform-specific ones involved hacking the Kconfig and
> commenting out a bunch of arch-dependent code to get the rest to
> compile), no runtime testing at all.
>
> Let me know if I missed any. Even better, let me know if we can just
> delete some of these entirely.
>
It's all downhill from here, Omar, I don't see how you are ever going to
top this in terms of impact.
Applied for 4.12!
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH 2/2] blk-stat: add a poll_size value to the request_queue struct
From: Jens Axboe @ 2017-03-28 20:38 UTC (permalink / raw)
To: Stephen Bates, Sagi Grimberg
Cc: linux-block@vger.kernel.org, Damien.LeMoal@wdc.com,
osandov@osandov.com, linux-nvme@lists.infradead.org
In-Reply-To: <C52E2CE3-0F92-40EA-902F-A9B70B145F1A@raithlin.com>
On 03/28/2017 01:58 PM, Stephen Bates wrote:
>>>
>>> Thanks for the review Sagi. I’d be OK going with <=0 as the exact
>>> match would normally be for minimal IO sizes (where <= and = are the
>>> same thing). I will see what other feedback I get and aim to do a
>>> respin soon…
>>
>> No tunables for this, please. There's absolutely no reason why we should
>> need it.
>
> Jens – by this you mean you want to only bucket IO that are exactly
> the minimum block size supported by the underlying block device? I was
> envisioning we might want to relax that in certain cases (e.g. bucket
> 4KB and below going to a 512B device).
Sorry, the above was a bit terse. I think a much better solution would
be to create a number of buckets (per data direction) and do stats on
all of them. The buckets would cover a reasonable range of request
sizes. Then when you poll for a given request, we can base our timed
number on the data direction AND size of it. You can get pretty far with
a few buckets:
512b
4k
8k
16k
32k
64k
128k
and you could even have your time estimation function turn these into
something sane. Or just use a composite of buckets, if you wish.
--
Jens Axboe
^ permalink raw reply
* [PATCH] block-mq: don't re-queue if we get a queue error
From: Josef Bacik @ 2017-03-28 20:37 UTC (permalink / raw)
To: linux-block, kernel-team
When try to issue a request directly and we fail we will requeue the
request, but call blk_mq_end_request() as well. This leads to the
completed request being on a queuelist and getting ended twice, which
causes list corruption in schedulers and other shenanigans.
Signed-off-by: Josef Bacik <jbacik@fb.com>
---
block/blk-mq.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 08a49c6..c5a6985 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1457,8 +1457,6 @@ static void blk_mq_try_issue_directly(struct request *rq, blk_qc_t *cookie,
return;
}
- __blk_mq_requeue_request(rq);
-
if (ret == BLK_MQ_RQ_QUEUE_ERROR) {
*cookie = BLK_QC_T_NONE;
rq->errors = -EIO;
@@ -1466,6 +1464,7 @@ static void blk_mq_try_issue_directly(struct request *rq, blk_qc_t *cookie,
return;
}
+ __blk_mq_requeue_request(rq);
insert:
blk_mq_sched_insert_request(rq, false, true, false, may_sleep);
}
--
2.7.4
^ permalink raw reply related
* Re: [PATCH 2/2] blk-stat: add a poll_size value to the request_queue struct
From: Stephen Bates @ 2017-03-28 19:58 UTC (permalink / raw)
To: Jens Axboe, Sagi Grimberg
Cc: linux-block@vger.kernel.org, Damien.LeMoal@wdc.com,
osandov@osandov.com, linux-nvme@lists.infradead.org
In-Reply-To: <88815dc6-04b2-0915-597d-1e335b6d2ba2@kernel.dk>
Pj4gDQo+PiBUaGFua3MgZm9yIHRoZSByZXZpZXcgU2FnaS4gSeKAmWQgYmUgT0sgZ29pbmcgd2l0
aCA8PTAgYXMgdGhlIGV4YWN0DQo+PiBtYXRjaCB3b3VsZCBub3JtYWxseSBiZSBmb3IgbWluaW1h
bCBJTyBzaXplcyAod2hlcmUgPD0gYW5kID0gYXJlIHRoZQ0KPj4gc2FtZSB0aGluZykuIEkgd2ls
bCBzZWUgd2hhdCBvdGhlciBmZWVkYmFjayBJIGdldCBhbmQgYWltIHRvIGRvIGENCj4+IHJlc3Bp
biBzb29u4oCmDQo+DQo+IE5vIHR1bmFibGVzIGZvciB0aGlzLCBwbGVhc2UuIFRoZXJlJ3MgYWJz
b2x1dGVseSBubyByZWFzb24gd2h5IHdlIHNob3VsZA0KPiBuZWVkIGl0Lg0KDQpKZW5zIOKAkyBi
eSB0aGlzIHlvdSBtZWFuIHlvdSB3YW50IHRvIG9ubHkgYnVja2V0IElPIHRoYXQgYXJlIGV4YWN0
bHkgdGhlIG1pbmltdW0gYmxvY2sgc2l6ZSBzdXBwb3J0ZWQgYnkgdGhlIHVuZGVybHlpbmcgYmxv
Y2sgZGV2aWNlPyBJIHdhcyBlbnZpc2lvbmluZyB3ZSBtaWdodCB3YW50IHRvIHJlbGF4IHRoYXQg
aW4gY2VydGFpbiBjYXNlcyAoZS5nLiBidWNrZXQgNEtCIGFuZCBiZWxvdyBnb2luZyB0byBhIDUx
MkIgZGV2aWNlKS4NCg0KU3RlcGhlbg0KDQoNCg==
^ permalink raw reply
* Re: [PATCH 2/2] blk-stat: add a poll_size value to the request_queue struct
From: Jens Axboe @ 2017-03-28 19:46 UTC (permalink / raw)
To: Stephen Bates, Sagi Grimberg
Cc: linux-block@vger.kernel.org, Damien.LeMoal@wdc.com,
osandov@osandov.com, linux-nvme@lists.infradead.org
In-Reply-To: <25CE7F43-DD1B-42D6-A891-A3D916BD0877@raithlin.com>
On 03/28/2017 01:38 PM, Stephen Bates wrote:
>>>
>>> In order to bucket IO for the polling algorithm we use a sysfs entry
>>> to set the filter value. It is signed and we will use that as follows:
>>>
>>> 0 : No filtering. All IO are considered in stat generation
>>> > 0 : Filtering based on IO of exactly this size only.
>>> < 0 : Filtering based on IO less than or equal to -1 time this value.>
>>
>> I'd say that this is a fairly non-trivial semantic meanning to this...
>>
>> Is there any use for the size exact match filter? If not then
>> I suggest we always make it (<=) in its semantics...
>
> Thanks for the review Sagi. I�d be OK going with <=0 as the exact
> match would normally be for minimal IO sizes (where <= and = are the
> same thing). I will see what other feedback I get and aim to do a
> respin soon�
No tunables for this, please. There's absolutely no reason why we should
need it.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH 2/2] blk-stat: add a poll_size value to the request_queue struct
From: Stephen Bates @ 2017-03-28 19:38 UTC (permalink / raw)
To: Sagi Grimberg, axboe@kernel.dk
Cc: linux-block@vger.kernel.org, Damien.LeMoal@wdc.com,
osandov@osandov.com, linux-nvme@lists.infradead.org
In-Reply-To: <44296e0a-fb04-888b-aa00-a9b01709e1b8@grimberg.me>
Pj4NCj4+ICBJbiBvcmRlciB0byBidWNrZXQgSU8gZm9yIHRoZSBwb2xsaW5nIGFsZ29yaXRobSB3
ZSB1c2UgYSBzeXNmcyBlbnRyeQ0KPj4gIHRvIHNldCB0aGUgZmlsdGVyIHZhbHVlLiBJdCBpcyBz
aWduZWQgYW5kIHdlIHdpbGwgdXNlIHRoYXQgYXMgZm9sbG93czoNCj4+DQo+PiAgIDAgICA6IE5v
IGZpbHRlcmluZy4gQWxsIElPIGFyZSBjb25zaWRlcmVkIGluIHN0YXQgZ2VuZXJhdGlvbg0KPj4g
ICA+IDAgOiBGaWx0ZXJpbmcgYmFzZWQgb24gSU8gb2YgZXhhY3RseSB0aGlzIHNpemUgb25seS4N
Cj4+ICAgPCAwIDogRmlsdGVyaW5nIGJhc2VkIG9uIElPIGxlc3MgdGhhbiBvciBlcXVhbCB0byAt
MSB0aW1lIHRoaXMgdmFsdWUuPg0KPg0KPiBJJ2Qgc2F5IHRoYXQgdGhpcyBpcyBhIGZhaXJseSBu
b24tdHJpdmlhbCBzZW1hbnRpYyBtZWFubmluZyB0byB0aGlzLi4uDQo+DQo+IElzIHRoZXJlIGFu
eSB1c2UgZm9yIHRoZSBzaXplIGV4YWN0IG1hdGNoIGZpbHRlcj8gSWYgbm90IHRoZW4NCj4gSSBz
dWdnZXN0IHdlIGFsd2F5cyBtYWtlIGl0ICg8PSkgaW4gaXRzIHNlbWFudGljcy4uLg0KDQpUaGFu
a3MgZm9yIHRoZSByZXZpZXcgU2FnaS4gSeKAmWQgYmUgT0sgZ29pbmcgd2l0aCA8PTAgYXMgdGhl
IGV4YWN0IG1hdGNoIHdvdWxkIG5vcm1hbGx5IGJlIGZvciBtaW5pbWFsIElPIHNpemVzICh3aGVy
ZSA8PSBhbmQgPSBhcmUgdGhlIHNhbWUgdGhpbmcpLiBJIHdpbGwgc2VlIHdoYXQgb3RoZXIgZmVl
ZGJhY2sgSSBnZXQgYW5kIGFpbSB0byBkbyBhIHJlc3BpbiBzb29u4oCmDQoNClN0ZXBoZW4NCg0K
^ permalink raw reply
* Re: [PATCH 03/23] sd: implement REQ_OP_WRITE_ZEROES
From: Mike Snitzer @ 2017-03-28 19:33 UTC (permalink / raw)
To: Bart Van Assche
Cc: agk@redhat.com, lars.ellenberg@linbit.com, hch@lst.de,
martin.petersen@oracle.com, philipp.reisner@linbit.com,
axboe@kernel.dk, shli@kernel.org, linux-scsi@vger.kernel.org,
dm-devel@redhat.com, drbd-dev@lists.linbit.com,
linux-block@vger.kernel.org, linux-raid@vger.kernel.org
In-Reply-To: <1490726988.2573.16.camel@sandisk.com>
On Tue, Mar 28 2017 at 2:50pm -0400,
Bart Van Assche <Bart.VanAssche@sandisk.com> wrote:
> On Thu, 2017-03-23 at 10:33 -0400, Christoph Hellwig wrote:
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > index af632e350ab4..b6f70a09a301 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -748,7 +748,7 @@ static int sd_setup_unmap_cmnd(struct scsi_cmnd *cmd)
> > return scsi_init_io(cmd);
> > }
> >
> > -static int sd_setup_write_same16_cmnd(struct scsi_cmnd *cmd)
> > +static int sd_setup_write_same16_cmnd(struct scsi_cmnd *cmd, bool unmap)
> > {
> > struct scsi_device *sdp = cmd->device;
> > struct request *rq = cmd->request;
> > @@ -765,13 +765,14 @@ static int sd_setup_write_same16_cmnd(struct scsi_cmnd *cmd)
> >
> > cmd->cmd_len = 16;
> > cmd->cmnd[0] = WRITE_SAME_16;
> > - cmd->cmnd[1] = 0x8; /* UNMAP */
> > + if (unmap)
> > + cmd->cmnd[1] = 0x8; /* UNMAP */
> > put_unaligned_be64(sector, &cmd->cmnd[2]);
> > put_unaligned_be32(nr_sectors, &cmd->cmnd[10]);
>
> Hello Christoph,
>
> A quote from SBC: "An OPTIMAL UNMAP GRANULARITY field set to a non-zero value
> indicates the optimal granularity in logical blocks for unmap requests (e.g.,
> an UNMAP command or a WRITE SAME (16) command with the UNMAP bit set to one).
> An unmap request with a number of logical blocks that is not a multiple of
> this value may result in unmap operations on fewer LBAs than requested."
>
> This means that just like the start and end of a discard must be aligned on a
> discard_granularity boundary, WRITE SAME commands with the UNMAP bit set must
> also respect that granularity. I think this means that either
> __blkdev_issue_zeroout() has to be modified such that it rejects unaligned
> REQ_OP_WRITE_ZEROES operations or that blk_bio_write_same_split() has to be
> modified such that it generates REQ_OP_WRITEs for the unaligned start and tail.
That'd get DM thinp off the hook from having to zero the unaligned start
and tail...
^ permalink raw reply
* Re: [PATCH 03/23] sd: implement REQ_OP_WRITE_ZEROES
From: Bart Van Assche @ 2017-03-28 18:50 UTC (permalink / raw)
To: agk@redhat.com, lars.ellenberg@linbit.com, snitzer@redhat.com,
hch@lst.de, martin.petersen@oracle.com,
philipp.reisner@linbit.com, axboe@kernel.dk, shli@kernel.org
Cc: linux-scsi@vger.kernel.org, dm-devel@redhat.com,
drbd-dev@lists.linbit.com, linux-block@vger.kernel.org,
linux-raid@vger.kernel.org
In-Reply-To: <20170323143341.31549-4-hch@lst.de>
On Thu, 2017-03-23 at 10:33 -0400, Christoph Hellwig wrote:
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index af632e350ab4..b6f70a09a301 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -748,7 +748,7 @@ static int sd_setup_unmap_cmnd(struct scsi_cmnd *cmd)
> return scsi_init_io(cmd);
> }
> =20
> -static int sd_setup_write_same16_cmnd(struct scsi_cmnd *cmd)
> +static int sd_setup_write_same16_cmnd(struct scsi_cmnd *cmd, bool unmap)
> {
> struct scsi_device *sdp =3D cmd->device;
> struct request *rq =3D cmd->request;
> @@ -765,13 +765,14 @@ static int sd_setup_write_same16_cmnd(struct scsi_c=
mnd *cmd)
> =20
> cmd->cmd_len =3D 16;
> cmd->cmnd[0] =3D WRITE_SAME_16;
> - cmd->cmnd[1] =3D 0x8; /* UNMAP */
> + if (unmap)
> + cmd->cmnd[1] =3D 0x8; /* UNMAP */
> put_unaligned_be64(sector, &cmd->cmnd[2]);
> put_unaligned_be32(nr_sectors, &cmd->cmnd[10]);
Hello Christoph,
A quote from SBC: "An OPTIMAL UNMAP GRANULARITY field set to a non-zero val=
ue
indicates the optimal granularity in logical blocks for unmap requests (e.g=
.,
an UNMAP command or a WRITE SAME (16) command with the UNMAP bit set to one=
).
An unmap request with a number of logical blocks that is not a multiple of
this value may result in unmap operations on fewer LBAs than requested."
This means that just like the start and end of a discard must be aligned on=
a
discard_granularity boundary, WRITE SAME commands with the UNMAP bit set mu=
st
also respect that granularity. I think this means that either
__blkdev_issue_zeroout() has to be modified such that it rejects unaligned
REQ_OP_WRITE_ZEROES operations or that blk_bio_write_same_split() has to be
modified such that it generates REQ_OP_WRITEs for the unaligned start and t=
ail.
Bart.=
^ permalink raw reply
* Re: [PATCH 23/23] block: remove the discard_zeroes_data flag
From: Bart Van Assche @ 2017-03-28 17:00 UTC (permalink / raw)
To: agk@redhat.com, lars.ellenberg@linbit.com, snitzer@redhat.com,
hch@lst.de, martin.petersen@oracle.com,
philipp.reisner@linbit.com, axboe@kernel.dk, shli@kernel.org
Cc: linux-scsi@vger.kernel.org, dm-devel@redhat.com,
drbd-dev@lists.linbit.com, linux-block@vger.kernel.org,
linux-raid@vger.kernel.org
In-Reply-To: <20170323143341.31549-24-hch@lst.de>
On Thu, 2017-03-23 at 10:33 -0400, Christoph Hellwig wrote:
> Now that we use the proper REQ_OP_WRITE_ZEROES operation everywhere we ca=
n
> kill this hack.
>=20
> [ ... ]
>
> diff --git a/Documentation/ABI/testing/sysfs-block b/Documentation/ABI/te=
sting/sysfs-block
> index 2da04ce6aeef..dea212db9df3 100644
> --- a/Documentation/ABI/testing/sysfs-block
> +++ b/Documentation/ABI/testing/sysfs-block
> @@ -213,14 +213,8 @@ What: /sys/block/<disk>/queue/discard_zeroes_data
> Date: May 2011
> Contact: Martin K. Petersen <martin.petersen@oracle.com>
> Description:
> - Devices that support discard functionality may return
> - stale or random data when a previously discarded block
> - is read back. This can cause problems if the filesystem
> - expects discarded blocks to be explicitly cleared. If a
> - device reports that it deterministically returns zeroes
> - when a discarded area is read the discard_zeroes_data
> - parameter will be set to one. Otherwise it will be 0 and
> - the result of reading a discarded area is undefined.
> + Will always return 0. Don't rely on any specific behavior
> + for discards, and don't read this file.
> =20
> What: /sys/block/<disk>/queue/write_same_max_bytes
> Date: January 2012
>
> [ ... ]
>
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -208,7 +208,7 @@ static ssize_t queue_discard_max_store(struct request=
_queue *q,
> =20
> static ssize_t queue_discard_zeroes_data_show(struct request_queue *q, c=
har *page)
> {
> - return queue_var_show(queue_discard_zeroes_data(q), page);
> + return 0;
> }
Hello Christoph,
It seems to me like the documentation in Documentation/ABI/testing/sysfs-bl=
ock
and the above code are not in sync. I think the above code will cause readi=
ng
from the discard_zeroes_data attribute to return an empty string ("") inste=
ad
of "0\n".
BTW, my personal preference is to remove this attribute entirely because ke=
eping
it will cause confusion, no matter how well we document the behavior of thi=
s
attribute.
Thanks,
Bart.=
^ permalink raw reply
* Re: [PATCH 11/23] block_dev: use blkdev_issue_zerout for hole punches
From: Bart Van Assche @ 2017-03-28 16:50 UTC (permalink / raw)
To: agk@redhat.com, lars.ellenberg@linbit.com, snitzer@redhat.com,
hch@lst.de, martin.petersen@oracle.com,
philipp.reisner@linbit.com, axboe@kernel.dk, shli@kernel.org
Cc: linux-scsi@vger.kernel.org, dm-devel@redhat.com,
drbd-dev@lists.linbit.com, linux-block@vger.kernel.org,
linux-raid@vger.kernel.org
In-Reply-To: <20170323143341.31549-12-hch@lst.de>
On Thu, 2017-03-23 at 10:33 -0400, Christoph Hellwig wrote:
> This gets us support for non-discard efficient write of zeroes (e.g. NVMe=
)
> and preparse for removing the discard_zeroes_data flag.
Hello Christoph,
"preparse" probably should have been "prepare"?
Thanks,
Bart.=
^ permalink raw reply
* Re: [PATCH 12/23] sd: handle REQ_UNMAP
From: Bart Van Assche @ 2017-03-28 16:48 UTC (permalink / raw)
To: agk@redhat.com, lars.ellenberg@linbit.com, snitzer@redhat.com,
hch@lst.de, martin.petersen@oracle.com,
philipp.reisner@linbit.com, axboe@kernel.dk, shli@kernel.org
Cc: linux-scsi@vger.kernel.org, dm-devel@redhat.com,
drbd-dev@lists.linbit.com, linux-block@vger.kernel.org,
linux-raid@vger.kernel.org
In-Reply-To: <20170323143341.31549-13-hch@lst.de>
On Thu, 2017-03-23 at 10:33 -0400, Christoph Hellwig wrote:
> Try to use a write same with unmap bit variant if the device supports it
> and the caller asks for it.
>=20
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/scsi/sd.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>=20
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index b6f70a09a301..ca96bb33471b 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -871,6 +871,16 @@ static int sd_setup_write_zeroes_cmnd(struct scsi_cm=
nd *cmd)
> return BLKPREP_INVALID;
> return sd_setup_ata_trim_cmnd(cmd);
> }
> +
> + if (rq->cmd_flags & REQ_UNMAP) {
> + switch (sdkp->provisioning_mode) {
> + case SD_LBP_WS16:
> + return sd_setup_write_same16_cmnd(cmd, true);
> + case SD_LBP_WS10:
> + return sd_setup_write_same10_cmnd(cmd, true);
> + }
> + }
> +
> if (sdp->no_write_same)
> return BLKPREP_INVALID;
> if (sdkp->ws16 || sector > 0xffffffff || nr_sectors > 0xffff)
Users can change the provisioning mode from user space from=A0SD_LBP_WS16 i=
nto
SD_LBP_WS10 so I'm not sure it's safe to skip the (sdkp->ws16 || sector >
0xffffffff || nr_sectors > 0xffff) check if REQ_UNMAP is set.
Bart.=
^ permalink raw reply
* Re: v4.11-rc blk-mq lockup?
From: Jens Axboe @ 2017-03-28 16:30 UTC (permalink / raw)
To: Bart Van Assche; +Cc: linux-block@vger.kernel.org
In-Reply-To: <1490718332.2573.6.camel@sandisk.com>
On 03/28/2017 10:25 AM, Bart Van Assche wrote:
> On Tue, 2017-03-28 at 08:06 -0600, Jens Axboe wrote:
>> On Mon, Mar 27 2017, Bart Van Assche wrote:
>>> Hello Jens,
>>>
>>> If I leave the srp-test software running for a few minutes using the
>>> following command:
>>>
>>> # while ~bart/software/infiniband/srp-test/run_tests -d -r 30; do :; done
>>>
>>> then after some time the following complaint appears for multiple
>>> kworkers:
>>>
>>> INFO: task kworker/9:0:65 blocked for more than 480 seconds.
>>> Tainted: G I 4.11.0-rc4-dbg+ #5
>>> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
>>> kworker/9:0 D 0 65 2 0x00000000
>>> Workqueue: dio/dm-0 dio_aio_complete_work
>>> Call Trace:
>>> __schedule+0x3df/0xc10
>>> schedule+0x38/0x90
>>> rwsem_down_write_failed+0x2c4/0x4c0
>>> call_rwsem_down_write_failed+0x17/0x30
>>> down_write+0x5a/0x70
>>> __generic_file_fsync+0x43/0x90
>>> ext4_sync_file+0x2d0/0x550
>>> vfs_fsync_range+0x46/0xa0
>>> dio_complete+0x181/0x1b0
>>> dio_aio_complete_work+0x17/0x20
>>> process_one_work+0x208/0x6a0
>>> worker_thread+0x49/0x4a0
>>> kthread+0x107/0x140
>>> ret_from_fork+0x2e/0x40
>>>
>>> I had not yet observed this behavior with kernel v4.10 or older. If this
>>> happens and I check the queue state with the following script:
>>
>> Can you include the 'state' file in your script?
>>
>> Do you know when this started happening? You say it doesn't happen in
>> 4.10, but did it pass earlier in the 4.11-rc cycle?
>>
>> Does it reproduce with dm?
>>
>> I can't tell from your report if this is new in the 4.11 series,
>>
>>> The kernel tree I used in my tests is the result of merging the
>>> following commits:
>>> * commit 3dca2c2f3d3b from git://git.kernel.dk/linux-block.git
>>> ("Merge branch 'for-4.12/block' into for-next")
>>> * commit f88ab0c4b481 from git://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git
>>> ("scsi: libsas: fix ata xfer length")
>>> * commit ad0376eb1483 from git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
>>> ("Merge tag 'edac_for_4.11_2' of git://git.kernel.org/pub/scm/linux/kernel/git/bp/bp")
>>
>> Can we try and isolate it a bit - -rc4 alone, for instance?
>
> Hello Jens,
>
> Sorry but performing a bisect would be hard: without recent SCSI and block
> layer fixes this test triggers other failures before the lockup reported in
> this e-mail is triggered. See e.g.
> https://marc.info/?l=linux-scsi&m=148979716822799.
Yeah, I realize that. Not necessarily a huge problem. If I can reproduce
it here, then I can poke enough at it to find out wtf is going on here.
> I do not know whether it would be possible to modify the test such that only
> the dm driver is involved but no SCSI code.
How about the other way around? Just SCSI, but no dm?
> When I reran the test this morning the hang was triggered by the 02-sq-on-mq
> test. This means that dm was used in blk-sq mode and that blk-mq was used for
> the ib_srp SCSI device instances.
>
> Please find below the updated script and its output.
Thanks for running it again, but it's the wrong state file. I should have
been more clear. The one I'm interested in is in the mq/<num>/ directories,
like the 'tags' etc files.
>
> ---
>
> #!/bin/bash
>
> show_state() {
> local a dev=$1
>
> for a in device/state queue/scheduler; do
> [ -e "$dev/$a" ] && grep -aH '' "$dev/$a"
> done
> }
>
> cd /sys/class/block || exit $?
> for dev in *; do
> if [ -e "$dev/mq" ]; then
> echo "$dev"
> pending=0
> for f in "$dev"/mq/*/{pending,*/rq_list}; do
> [ -e "$f" ] || continue
> if { read -r line1 && read -r line2; } <"$f"; then
> echo "$f"
> echo "$line1 $line2" >/dev/null
> head -n 9 "$f"
> ((pending++))
> fi
> done
> (
> busy=0
> cd /sys/kernel/debug/block >&/dev/null &&
> for d in "$dev"/mq/*; do
> [ ! -d "$d" ] && continue
> grep -q '^busy=0$' "$d/tags" && continue
> ((busy++))
> for f in "$d"/{dispatch,tags*,cpu*/rq_list}; do
Ala:
for f in "$d"/{dispatch,state,tags*,cpu*/rq_list}; do
Also, can you include the involved dm devices as well for this state
dump?
--
Jens Axboe
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox