* [PATCH] nbd: use bdget_disk to get bdev
From: Josef Bacik @ 2017-02-13 17:31 UTC (permalink / raw)
To: axboe, nbd-general, linux-block, kernel-team
In preparation for the upcoming netlink interface we need to not rely on
already having the bdev for the NBD device we are doing operations on.
Instead of passing the bdev from the bdev ioctl around, use bdget_disk()
wherever we need the bdev and pass that down to the helpers as
necessary.
Signed-off-by: Josef Bacik <jbacik@fb.com>
---
drivers/block/nbd.c | 88 ++++++++++++++++++++++++++++++-----------------------
1 file changed, 50 insertions(+), 38 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 25891a1..0623f8f 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -168,13 +168,18 @@ static void nbd_size_update(struct nbd_device *nbd, struct block_device *bdev)
kobject_uevent(&nbd_to_dev(nbd)->kobj, KOBJ_CHANGE);
}
-static void nbd_size_set(struct nbd_device *nbd, struct block_device *bdev,
- loff_t blocksize, loff_t nr_blocks)
+static int nbd_size_set(struct nbd_device *nbd, loff_t blocksize,
+ loff_t nr_blocks)
{
+ struct block_device *bdev = bdget_disk(nbd->disk, 0);
+ if (!bdev)
+ return -EINVAL;
nbd->blksize = blocksize;
nbd->bytesize = blocksize * nr_blocks;
if (nbd_is_connected(nbd))
nbd_size_update(nbd, bdev);
+ bdput(bdev);
+ return 0;
}
static void nbd_end_request(struct nbd_cmd *cmd)
@@ -704,8 +709,7 @@ static int nbd_wait_for_socks(struct nbd_device *nbd)
return ret;
}
-static int nbd_add_socket(struct nbd_device *nbd, struct block_device *bdev,
- unsigned long arg)
+static int nbd_add_socket(struct nbd_device *nbd, unsigned long arg)
{
struct socket *sock;
struct nbd_sock **socks;
@@ -749,8 +753,6 @@ static int nbd_add_socket(struct nbd_device *nbd, struct block_device *bdev,
nsock->sock = sock;
socks[nbd->num_connections++] = nsock;
- if (max_part)
- bdev->bd_invalidated = 1;
err = 0;
out:
mutex_unlock(&nbd->socks_lock);
@@ -771,18 +773,19 @@ static void nbd_reset(struct nbd_device *nbd)
static void nbd_bdev_reset(struct block_device *bdev)
{
- set_device_ro(bdev, false);
- bdev->bd_inode->i_size = 0;
+ bd_set_size(bdev, 0);
if (max_part > 0) {
blkdev_reread_part(bdev);
bdev->bd_invalidated = 1;
}
}
-static void nbd_parse_flags(struct nbd_device *nbd, struct block_device *bdev)
+static void nbd_parse_flags(struct nbd_device *nbd)
{
if (nbd->flags & NBD_FLAG_READ_ONLY)
- set_device_ro(bdev, true);
+ set_disk_ro(nbd->disk, true);
+ else
+ set_disk_ro(nbd->disk, false);
if (nbd->flags & NBD_FLAG_SEND_TRIM)
queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
if (nbd->flags & NBD_FLAG_SEND_FLUSH)
@@ -807,16 +810,11 @@ static void send_disconnects(struct nbd_device *nbd)
}
}
-static int nbd_disconnect(struct nbd_device *nbd, struct block_device *bdev)
+static int nbd_disconnect(struct nbd_device *nbd)
{
dev_info(disk_to_dev(nbd->disk), "NBD_DISCONNECT\n");
if (!nbd_socks_get_unless_zero(nbd))
return -EINVAL;
-
- mutex_unlock(&nbd->config_lock);
- fsync_bdev(bdev);
- mutex_lock(&nbd->config_lock);
-
if (!test_and_set_bit(NBD_DISCONNECT_REQUESTED,
&nbd->runtime_flags))
send_disconnects(nbd);
@@ -824,28 +822,40 @@ static int nbd_disconnect(struct nbd_device *nbd, struct block_device *bdev)
return 0;
}
-static int nbd_clear_sock(struct nbd_device *nbd, struct block_device *bdev)
+static int nbd_clear_sock(struct nbd_device *nbd)
{
+ struct block_device *bdev = bdget_disk(nbd->disk, 0);
+
sock_shutdown(nbd);
nbd_clear_que(nbd);
- kill_bdev(bdev);
- nbd_bdev_reset(bdev);
+ if (bdev) {
+ kill_bdev(bdev);
+ nbd_bdev_reset(bdev);
+ bdput(bdev);
+ }
nbd->task_setup = NULL;
if (test_and_clear_bit(NBD_HAS_SOCKS_REF, &nbd->runtime_flags))
nbd_socks_put(nbd);
return 0;
}
-static int nbd_start_device(struct nbd_device *nbd, struct block_device *bdev)
+static int nbd_start_device(struct nbd_device *nbd)
{
struct recv_thread_args *args;
+ struct block_device *bdev = bdget_disk(nbd->disk, 0);
int num_connections = nbd->num_connections;
int error = 0, i;
- if (nbd->task_recv)
+ if (!bdev)
+ return -EINVAL;
+ if (nbd->task_recv) {
+ bdput(bdev);
return -EBUSY;
- if (!nbd->socks)
+ }
+ if (!nbd->socks) {
+ bdput(bdev);
return -EINVAL;
+ }
if (num_connections > 1 &&
!(nbd->flags & NBD_FLAG_CAN_MULTI_CONN)) {
dev_err(disk_to_dev(nbd->disk), "server does not support multiple connections per device.\n");
@@ -853,6 +863,9 @@ static int nbd_start_device(struct nbd_device *nbd, struct block_device *bdev)
goto out_err;
}
+ if (max_part)
+ bdev->bd_invalidated = 1;
+
blk_mq_update_nr_hw_queues(&nbd->tag_set, nbd->num_connections);
args = kcalloc(num_connections, sizeof(*args), GFP_KERNEL);
if (!args) {
@@ -862,7 +875,7 @@ static int nbd_start_device(struct nbd_device *nbd, struct block_device *bdev)
nbd->task_recv = current;
mutex_unlock(&nbd->config_lock);
- nbd_parse_flags(nbd, bdev);
+ nbd_parse_flags(nbd);
error = device_create_file(disk_to_dev(nbd->disk), &pid_attr);
if (error) {
@@ -893,7 +906,7 @@ static int nbd_start_device(struct nbd_device *nbd, struct block_device *bdev)
mutex_lock(&nbd->config_lock);
nbd->task_recv = NULL;
out_err:
- nbd_clear_sock(nbd, bdev);
+ nbd_clear_sock(nbd);
/* user requested, ignore socket errors */
if (test_bit(NBD_DISCONNECT_REQUESTED, &nbd->runtime_flags))
@@ -902,31 +915,30 @@ static int nbd_start_device(struct nbd_device *nbd, struct block_device *bdev)
error = -ETIMEDOUT;
nbd_reset(nbd);
+ bdput(bdev);
return error;
}
/* Must be called with config_lock held */
-static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
- unsigned int cmd, unsigned long arg)
+static int __nbd_ioctl(struct nbd_device *nbd, unsigned int cmd,
+ unsigned long arg)
{
switch (cmd) {
case NBD_DISCONNECT:
- return nbd_disconnect(nbd, bdev);
+ return nbd_disconnect(nbd);
case NBD_CLEAR_SOCK:
- return nbd_clear_sock(nbd, bdev);
+ return nbd_clear_sock(nbd);
case NBD_SET_SOCK:
- return nbd_add_socket(nbd, bdev, arg);
+ return nbd_add_socket(nbd, arg);
case NBD_SET_BLKSIZE:
- nbd_size_set(nbd, bdev, arg,
- div_s64(nbd->bytesize, arg));
+ return nbd_size_set(nbd, arg,
+ div_s64(nbd->bytesize, arg));
return 0;
case NBD_SET_SIZE:
- nbd_size_set(nbd, bdev, nbd->blksize,
- div_s64(arg, nbd->blksize));
- return 0;
+ return nbd_size_set(nbd, nbd->blksize,
+ div_s64(arg, nbd->blksize));
case NBD_SET_SIZE_BLOCKS:
- nbd_size_set(nbd, bdev, nbd->blksize, arg);
- return 0;
+ return nbd_size_set(nbd, nbd->blksize, arg);
case NBD_SET_TIMEOUT:
nbd->tag_set.timeout = arg * HZ;
return 0;
@@ -935,7 +947,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
nbd->flags = arg;
return 0;
case NBD_DO_IT:
- return nbd_start_device(nbd, bdev);
+ return nbd_start_device(nbd);
case NBD_CLEAR_QUE:
/*
* This is for compatibility only. The queue is always cleared
@@ -964,7 +976,7 @@ static int nbd_ioctl(struct block_device *bdev, fmode_t mode,
BUG_ON(nbd->magic != NBD_MAGIC);
mutex_lock(&nbd->config_lock);
- error = __nbd_ioctl(bdev, nbd, cmd, arg);
+ error = __nbd_ioctl(nbd, cmd, arg);
mutex_unlock(&nbd->config_lock);
return error;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH V5 3/4] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN
From: Arnd Bergmann @ 2017-02-13 20:16 UTC (permalink / raw)
To: David Laight
Cc: Scott Bauer, linux-nvme@lists.infradead.org, axboe@fb.com,
keith.busch@intel.com, jonathan.derrick@intel.com,
hch@infradead.org, linux-kernel@vger.kernel.org,
linux-block@vger.kernel.org
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6DB02851D7@AcuExch.aculab.com>
On Mon, Feb 13, 2017 at 6:07 PM, David Laight <David.Laight@aculab.com> wrote:
> From: Arnd Bergmann
>> Sent: 13 February 2017 17:02
> ...
>> >> > + ioctl_ptr = memdup_user(arg, _IOC_SIZE(cmd));
>> >> > + if (IS_ERR_OR_NULL(ioctl_ptr)) {
>> >> > + ret = PTR_ERR(ioctl_ptr);
>> >> > + goto out;
>> >> ...
>> >> > + out:
>> >> > + kfree(ioctl_ptr);
>> >> > + return ret;
> ...
>> >> That error path doesn't look quite right to me.
> ...
>> > good god, this is a mess this morning. Thanks for the catch, I'll review these
>> > more aggressively before sending out.
>>
>> memdup_user() never returns NULL, and generally speaking any use of
>> IS_ERR_OR_NULL() indicates that there is something wrong with the
>> interface, so aside from passing the right pointer (or NULL) into kfree()
>> I think using IS_ERR() is the correct solution.
>
> You missed the problem entirely.
> Code needs to be:
> if (IS_ERR(ioctl_ptr))
> return PTR_ERR(ioctl_ptr);
Sorry if that wasn't clear, I expected the part about the kfree(errptr) to be
obvious but was trying to avoid having Scott go through another revision
for removing the IS_ERR_OR_NULL() after fixing the first problem.
Arnd
^ permalink raw reply
* [PATCH BUGFIX] attempt to fix wrong scheduler selection
From: Paolo Valente @ 2017-02-13 21:01 UTC (permalink / raw)
To: Jens Axboe, Tejun Heo
Cc: linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie,
Paolo Valente
Hi,
if, at boot, a legacy I/O scheduler is chosen for a device using
blk-mq, or, viceversa, a blk-mq scheduler is chosen for a device using
blk, then that scheduler is set and initialized without any check,
driving the system into an inconsistent state.
The purpose of this message is, first, to report this issue, and,
second, to propose a possible fix in case you do consider this as a
bug.
Thanks,
Paolo
Paolo Valente (1):
block: make elevator_get robust against cross blk/blk-mq choice
block/elevator.c | 26 ++++++++++++++++++--------
1 file changed, 18 insertions(+), 8 deletions(-)
--
2.10.0
^ permalink raw reply
* [PATCH BUGFIX] block: make elevator_get robust against cross blk/blk-mq choice
From: Paolo Valente @ 2017-02-13 21:01 UTC (permalink / raw)
To: Jens Axboe, Tejun Heo
Cc: linux-block, linux-kernel, ulf.hansson, linus.walleij, broonie,
Paolo Valente
In-Reply-To: <20170213210107.4848-1-paolo.valente@linaro.org>
If, at boot, a legacy I/O scheduler is chosen for a device using blk-mq,
or, viceversa, a blk-mq scheduler is chosen for a device using blk, then
that scheduler is set and initialized without any check, driving the
system into an inconsistent state. This commit addresses this issue by
letting elevator_get fail for these wrong cross choices.
Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
---
block/elevator.c | 26 ++++++++++++++++++--------
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/block/elevator.c b/block/elevator.c
index 27ff1ed..a25bdd9 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -99,7 +99,8 @@ static void elevator_put(struct elevator_type *e)
module_put(e->elevator_owner);
}
-static struct elevator_type *elevator_get(const char *name, bool try_loading)
+static struct elevator_type *elevator_get(const char *name, bool try_loading,
+ bool mq_ops)
{
struct elevator_type *e;
@@ -113,6 +114,12 @@ static struct elevator_type *elevator_get(const char *name, bool try_loading)
e = elevator_find(name);
}
+ if (e && (e->uses_mq != mq_ops)) {
+ pr_err("ERROR: attempted to choose %s %s I/O scheduler in blk%s",
+ name, e->uses_mq ? "blk-mq" : "legacy", mq_ops ? "-mq" : "");
+ e = NULL;
+ }
+
if (e && !try_module_get(e->elevator_owner))
e = NULL;
@@ -201,7 +208,7 @@ int elevator_init(struct request_queue *q, char *name)
q->boundary_rq = NULL;
if (name) {
- e = elevator_get(name, true);
+ e = elevator_get(name, true, q->mq_ops);
if (!e)
return -EINVAL;
}
@@ -212,7 +219,7 @@ int elevator_init(struct request_queue *q, char *name)
* off async and request_module() isn't allowed from async.
*/
if (!e && *chosen_elevator) {
- e = elevator_get(chosen_elevator, false);
+ e = elevator_get(chosen_elevator, false, q->mq_ops);
if (!e)
printk(KERN_ERR "I/O scheduler %s not found\n",
chosen_elevator);
@@ -220,17 +227,20 @@ int elevator_init(struct request_queue *q, char *name)
if (!e) {
if (q->mq_ops && q->nr_hw_queues == 1)
- e = elevator_get(CONFIG_DEFAULT_SQ_IOSCHED, false);
+ e = elevator_get(CONFIG_DEFAULT_SQ_IOSCHED, false,
+ q->mq_ops);
else if (q->mq_ops)
- e = elevator_get(CONFIG_DEFAULT_MQ_IOSCHED, false);
+ e = elevator_get(CONFIG_DEFAULT_MQ_IOSCHED, false,
+ q->mq_ops);
else
- e = elevator_get(CONFIG_DEFAULT_IOSCHED, false);
+ e = elevator_get(CONFIG_DEFAULT_IOSCHED, false,
+ q->mq_ops);
if (!e) {
printk(KERN_ERR
"Default I/O scheduler not found. " \
"Using noop/none.\n");
- e = elevator_get("noop", false);
+ e = elevator_get("noop", false, q->mq_ops);
}
}
@@ -1051,7 +1061,7 @@ static int __elevator_change(struct request_queue *q, const char *name)
return elevator_switch(q, NULL);
strlcpy(elevator_name, name, sizeof(elevator_name));
- e = elevator_get(strstrip(elevator_name), true);
+ e = elevator_get(strstrip(elevator_name), true, q->mq_ops);
if (!e) {
printk(KERN_ERR "elevator: type %s not found\n", elevator_name);
return -EINVAL;
--
2.10.0
^ permalink raw reply related
* Re: [WIP PATCHSET 0/4] WIP branch for bfq-mq
From: Paolo Valente @ 2017-02-13 21:07 UTC (permalink / raw)
To: Bart Van Assche
Cc: tj@kernel.org, axboe@kernel.dk, ulf.hansson@linaro.org,
linux-kernel@vger.kernel.org, linux-block@vger.kernel.org,
broonie@kernel.org, linus.walleij@linaro.org
In-Reply-To: <55E6D3EE-27D1-4D46-ABE5-750445FB941B@linaro.org>
> Il giorno 10 feb 2017, alle ore 20:49, Paolo Valente =
<paolo.valente@linaro.org> ha scritto:
>=20
>>=20
>> Il giorno 10 feb 2017, alle ore 19:13, Bart Van Assche =
<bart.vanassche@sandisk.com> ha scritto:
>>=20
>> On 02/10/2017 08:49 AM, Paolo Valente wrote:
>>>> $ grep '^C.*_MQ_' .config
>>>> CONFIG_BLK_MQ_PCI=3Dy
>>>> CONFIG_MQ_IOSCHED_BFQ=3Dy
>>>> CONFIG_MQ_IOSCHED_DEADLINE=3Dy
>>>> CONFIG_MQ_IOSCHED_NONE=3Dy
>>>> CONFIG_DEFAULT_MQ_BFQ_MQ=3Dy
>>>> CONFIG_DEFAULT_MQ_IOSCHED=3D"bfq-mq"
>>>> CONFIG_SCSI_MQ_DEFAULT=3Dy
>>>> CONFIG_DM_MQ_DEFAULT=3Dy
>>>>=20
>>>=20
>>> Could you reconfigure with none or mq-deadline as default, check
>>> whether the system boots, and, it it does, switch manually to =
bfq-mq,
>>> check what happens, and, in the likely case of a failure, try to get
>>> the oops?
>>=20
>> Hello Paolo,
>>=20
>> I just finished performing that test with the following kernel =
config:
>> $ grep '^C.*_MQ_' .config
>> CONFIG_BLK_MQ_PCI=3Dy
>> CONFIG_MQ_IOSCHED_BFQ=3Dy
>> CONFIG_MQ_IOSCHED_DEADLINE=3Dy
>> CONFIG_MQ_IOSCHED_NONE=3Dy
>> CONFIG_DEFAULT_MQ_DEADLINE=3Dy
>> CONFIG_DEFAULT_MQ_IOSCHED=3D"mq-deadline"
>> CONFIG_SCSI_MQ_DEFAULT=3Dy
>> CONFIG_DM_MQ_DEFAULT=3Dy
>>=20
>> After the system came up I logged in, switched to the bfq-mq =
scheduler
>> and ran several I/O tests against the boot disk.
>=20
> Without any failure, right?
>=20
> Unfortunately, as you can imagine, no boot failure occurred on
> any of my test systems so far :(
>=20
> This version of bfq-mq can be configured to print all its activity in
> the kernel log, by just defining a macro. This will of course slow
> down the system so much to make it probably unusable, if bfq-mq is
> active from boot. Yet, the failure may still occur so early to make
> this approach useful to discover where bfq-mq gets stuck. As of now I
> have no better ideas. Any suggestion is welcome.
>=20
Hi Bart,
I have found a machine crashing at boot, yet not only when bfq-mq is
chosen, but also when mq-deadline is chosen as the default
scheduler. I have found and just reported the cause of the failure,
together with a fix. Probably this is not the cause of your failure,
but what do you think about trying this fix? BTW, I have rebased the
branch [1] against the new commits in Jens for-4.11/next.
Otherwise, if you have no news or suggestions, would you be willing to
try my micro-logging proposal?
Thanks,
Paolo
[1] https://github.com/Algodev-github/bfq-mq
> Thanks,
> Paolo
>=20
>> Sorry but nothing
>> interesting appeared in the kernel log.
>>=20
>> Bart.
>> Western Digital Corporation (and its subsidiaries) E-mail =
Confidentiality Notice & Disclaimer:
>>=20
>> This e-mail and any files transmitted with it may contain =
confidential or legally privileged information of WDC and/or its =
affiliates, and are intended solely for the use of the individual or =
entity to which they are addressed. If you are not the intended =
recipient, any disclosure, copying, distribution or any action taken or =
omitted to be taken in reliance on it, is prohibited. If you have =
received this e-mail in error, please notify the sender immediately and =
delete the e-mail in its entirety from your system.
^ permalink raw reply
* Re: [WIP PATCHSET 0/4] WIP branch for bfq-mq
From: Paolo Valente @ 2017-02-13 21:09 UTC (permalink / raw)
To: Paolo Valente
Cc: Jens Axboe, Tejun Heo, linux-block, linux-kernel, ulf.hansson,
linus.walleij, broonie
In-Reply-To: <20170207172446.4528-1-paolo.valente@linaro.org>
> Il giorno 07 feb 2017, alle ore 18:24, Paolo Valente =
<paolo.valente@linaro.org> ha scritto:
>=20
> Hi,
>=20
> I have finally pushed here [1] the current WIP branch of bfq for
> blk-mq, which I have tentatively named bfq-mq.
>=20
> This branch *IS NOT* meant for merging into mainline and contain code
> that mau easily violate code style, and not only, in many
> places. Commits implement the following main steps:
> 1) Add the last version of bfq for blk
> 2) Clone bfq source files into identical bfq-mq source files
> 3) Modify bfq-mq files to get a working version of bfq for blk-mq
> (cgroups support not yet functional)
>=20
> In my intentions, the main goals of this branch are:
>=20
> 1) Show, as soon as I could, the changes I made to let bfq-mq comply
> with blk-mq-sched framework. I though this could be particularly
> useful for Jens, being BFQ identical to CFQ in terms of hook
> interfaces and io-context handling, and almost identical in terms
> request-merging.
>=20
> 2) Enable people to test this first version bfq-mq. Code is purposely
> overfull of log messages and invariant checks that halt the system on
> failure (lock assertions, BUG_ONs, ...).
>=20
> To make it easier to revise commits, I'm sending the patches that
> transform bfq into bfq-mq (last four patches in the branch [1]). They
> work on two files, bfq-mq-iosched.c and bfq-mq.h, which, at the
> beginning, are just copies of bfq-iosched.c and bfq.h.
>=20
Hi,
this is just to inform that, as I just wrote to Bart, I have rebase
the branch [1] against the current content of for-4.11/next.
Jens, Omar, did you find the time to have a look at the main commits
or to run some test?
Thanks,
Paolo
[1] https://github.com/Algodev-github/bfq-mq
> Thanks,
> Paolo
>=20
> [1] https://github.com/Algodev-github/bfq-mq
>=20
> Paolo Valente (4):
> blk-mq: pass bio to blk_mq_sched_get_rq_priv
> Move thinktime from bic to bfqq
> Embed bfq-ioc.c and add locking on request queue
> Modify interface and operation to comply with blk-mq-sched
>=20
> block/bfq-cgroup.c | 4 -
> block/bfq-mq-iosched.c | 852 =
+++++++++++++++++++++++++++++------------------
> block/bfq-mq.h | 65 ++--
> block/blk-mq-sched.c | 8 +-
> block/blk-mq-sched.h | 5 +-
> include/linux/elevator.h | 2 +-
> 6 files changed, 567 insertions(+), 369 deletions(-)
>=20
> --
> 2.10.0
^ permalink raw reply
* Re: [PATCH BUGFIX] block: make elevator_get robust against cross blk/blk-mq choice
From: Bart Van Assche @ 2017-02-13 21:12 UTC (permalink / raw)
To: tj@kernel.org, paolo.valente@linaro.org, axboe@kernel.dk
Cc: ulf.hansson@linaro.org, linux-kernel@vger.kernel.org,
linux-block@vger.kernel.org, broonie@kernel.org,
linus.walleij@linaro.org
In-Reply-To: <20170213210107.4848-2-paolo.valente@linaro.org>
On Mon, 2017-02-13 at 22:01 +0100, Paolo Valente wrote:
> -static struct elevator_type *elevator_get(const char *name, bool try_loa=
ding)
> +static struct elevator_type *elevator_get(const char *name, bool try_loa=
ding,
> + bool mq_ops)
Please choose a better name for that argument, e.q. "mq". To me the name "m=
q_ops"
means "a pointer to a data structure with operation function pointers".
> + if (e && (e->uses_mq !=3D mq_ops)) {
> + pr_err("ERROR: attempted to choose %s %s I/O scheduler in blk%s",
> + name, e->uses_mq ? "blk-mq" : "legacy", mq_ops ? "-mq" : "");
> + e =3D NULL;
> + }
How about changing the above into:
+=A0=A0=A0=A0=A0=A0=A0if (e && e->uses_mq !=3D mq) {
+=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0pr_err("ERROR: attempt to con=
figure %s as I/O scheduler for a %s queue\n",
+=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0name, mq=
? "blk-mq" : "legacy");
+=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0e =3D NULL;
+=A0=A0=A0=A0=A0=A0=A0}
Thanks,
Bart.
Western Digital Corporation (and its subsidiaries) E-mail Confidentiality N=
otice & Disclaimer:
This e-mail and any files transmitted with it may contain confidential or l=
egally privileged information of WDC and/or its affiliates, and are intende=
d solely for the use of the individual or entity to which they are addresse=
d. If you are not the intended recipient, any disclosure, copying, distribu=
tion or any action taken or omitted to be taken in reliance on it, is prohi=
bited. If you have received this e-mail in error, please notify the sender =
immediately and delete the e-mail in its entirety from your system.
^ permalink raw reply
* Re: [WIP PATCHSET 0/4] WIP branch for bfq-mq
From: Bart Van Assche @ 2017-02-13 22:07 UTC (permalink / raw)
To: paolo.valente@linaro.org
Cc: ulf.hansson@linaro.org, tj@kernel.org,
linux-kernel@vger.kernel.org, linux-block@vger.kernel.org,
broonie@kernel.org, linus.walleij@linaro.org, axboe@kernel.dk
In-Reply-To: <1A8DF6A8-C251-472A-9CE1-ACB1BCD0E9AB@linaro.org>
On Mon, 2017-02-13 at 22:07 +0100, Paolo Valente wrote:
> but what do you think about trying this fix?
Sorry but with ... the same server I used for the previous test still
didn't boot up properly. A screenshot is available at
https://goo.gl/photos/Za9QVGCNe2BJBwxVA.
> Otherwise, if you have no news or suggestions, would you be willing to
> try my micro-logging proposal https://github.com/Algodev-github/bfq-mq?
Sorry but it's not clear to me what logging mechanism you are referring
to and how to enable it? Are you perhaps referring to
CONFIG_BFQ_REDIRECT_TO_CONSOLE?
Thanks,
Bart.
Western Digital Corporation (and its subsidiaries) E-mail Confidentiality N=
otice & Disclaimer:
This e-mail and any files transmitted with it may contain confidential or l=
egally privileged information of WDC and/or its affiliates, and are intende=
d solely for the use of the individual or entity to which they are addresse=
d. If you are not the intended recipient, any disclosure, copying, distribu=
tion or any action taken or omitted to be taken in reliance on it, is prohi=
bited. If you have received this e-mail in error, please notify the sender =
immediately and delete the e-mail in its entirety from your system.
^ permalink raw reply
* Re: [PATCH BUGFIX] block: make elevator_get robust against cross blk/blk-mq choice
From: Omar Sandoval @ 2017-02-13 22:09 UTC (permalink / raw)
To: Paolo Valente
Cc: Jens Axboe, Tejun Heo, linux-block, linux-kernel, ulf.hansson,
linus.walleij, broonie
In-Reply-To: <20170213210107.4848-2-paolo.valente@linaro.org>
On Mon, Feb 13, 2017 at 10:01:07PM +0100, Paolo Valente wrote:
> If, at boot, a legacy I/O scheduler is chosen for a device using blk-mq,
> or, viceversa, a blk-mq scheduler is chosen for a device using blk, then
> that scheduler is set and initialized without any check, driving the
> system into an inconsistent state. This commit addresses this issue by
> letting elevator_get fail for these wrong cross choices.
>
> Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
> ---
> block/elevator.c | 26 ++++++++++++++++++--------
> 1 file changed, 18 insertions(+), 8 deletions(-)
Hey, Paolo,
How exactly are you triggering this? In __elevator_change(), we do check
for mq or not mq:
if (!e->uses_mq && q->mq_ops) {
elevator_put(e);
return -EINVAL;
}
if (e->uses_mq && !q->mq_ops) {
elevator_put(e);
return -EINVAL;
}
We don't ever appear to call elevator_init() with a specific scheduler
name, and for the default we switch off of q->mq_ops and use the
defaults from Kconfig:
if (q->mq_ops && q->nr_hw_queues == 1)
e = elevator_get(CONFIG_DEFAULT_SQ_IOSCHED, false);
else if (q->mq_ops)
e = elevator_get(CONFIG_DEFAULT_MQ_IOSCHED, false);
else
e = elevator_get(CONFIG_DEFAULT_IOSCHED, false);
if (!e) {
printk(KERN_ERR
"Default I/O scheduler not found. " \
"Using noop/none.\n");
e = elevator_get("noop", false);
}
So I guess this could happen if someone manually changed those Kconfig
options, but I don't see what other case would make this happen, could
you please explain?
^ permalink raw reply
* Re: [PATCH BUGFIX] block: make elevator_get robust against cross blk/blk-mq choice
From: Jens Axboe @ 2017-02-13 22:28 UTC (permalink / raw)
To: Omar Sandoval, Paolo Valente
Cc: Tejun Heo, linux-block, linux-kernel, ulf.hansson, linus.walleij,
broonie
In-Reply-To: <20170213220900.GA11052@vader.DHCP.thefacebook.com>
On 02/13/2017 03:09 PM, Omar Sandoval wrote:
> On Mon, Feb 13, 2017 at 10:01:07PM +0100, Paolo Valente wrote:
>> If, at boot, a legacy I/O scheduler is chosen for a device using blk-mq,
>> or, viceversa, a blk-mq scheduler is chosen for a device using blk, then
>> that scheduler is set and initialized without any check, driving the
>> system into an inconsistent state. This commit addresses this issue by
>> letting elevator_get fail for these wrong cross choices.
>>
>> Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
>> ---
>> block/elevator.c | 26 ++++++++++++++++++--------
>> 1 file changed, 18 insertions(+), 8 deletions(-)
>
> Hey, Paolo,
>
> How exactly are you triggering this? In __elevator_change(), we do check
> for mq or not mq:
>
> if (!e->uses_mq && q->mq_ops) {
> elevator_put(e);
> return -EINVAL;
> }
> if (e->uses_mq && !q->mq_ops) {
> elevator_put(e);
> return -EINVAL;
> }
>
> We don't ever appear to call elevator_init() with a specific scheduler
> name, and for the default we switch off of q->mq_ops and use the
> defaults from Kconfig:
>
> if (q->mq_ops && q->nr_hw_queues == 1)
> e = elevator_get(CONFIG_DEFAULT_SQ_IOSCHED, false);
> else if (q->mq_ops)
> e = elevator_get(CONFIG_DEFAULT_MQ_IOSCHED, false);
> else
> e = elevator_get(CONFIG_DEFAULT_IOSCHED, false);
>
> if (!e) {
> printk(KERN_ERR
> "Default I/O scheduler not found. " \
> "Using noop/none.\n");
> e = elevator_get("noop", false);
> }
>
> So I guess this could happen if someone manually changed those Kconfig
> options, but I don't see what other case would make this happen, could
> you please explain?
Was wondering the same - is it using the 'elevator=' boot parameter?
Didn't look at that path just now, but that's the only one I could
think of. If it is, I'd much prefer only using 'chosen_elevator' for
the non-mq stuff, and the fix should be just that instead.
So instead of:
if (!e && *chosen_elevator) {
do
if (!e && !q->mq_ops && && *chosen_elevator) {
--
Jens Axboe
^ permalink raw reply
* Re: [WIP PATCHSET 0/4] WIP branch for bfq-mq
From: Jens Axboe @ 2017-02-13 22:29 UTC (permalink / raw)
To: Paolo Valente
Cc: Tejun Heo, linux-block, linux-kernel, ulf.hansson, linus.walleij,
broonie
In-Reply-To: <478A4EBE-9218-4527-9F8A-42AAEBDC9470@linaro.org>
On 02/13/2017 02:09 PM, Paolo Valente wrote:
>
>> Il giorno 07 feb 2017, alle ore 18:24, Paolo Valente <paolo.valente@linaro.org> ha scritto:
>>
>> Hi,
>>
>> I have finally pushed here [1] the current WIP branch of bfq for
>> blk-mq, which I have tentatively named bfq-mq.
>>
>> This branch *IS NOT* meant for merging into mainline and contain code
>> that mau easily violate code style, and not only, in many
>> places. Commits implement the following main steps:
>> 1) Add the last version of bfq for blk
>> 2) Clone bfq source files into identical bfq-mq source files
>> 3) Modify bfq-mq files to get a working version of bfq for blk-mq
>> (cgroups support not yet functional)
>>
>> In my intentions, the main goals of this branch are:
>>
>> 1) Show, as soon as I could, the changes I made to let bfq-mq comply
>> with blk-mq-sched framework. I though this could be particularly
>> useful for Jens, being BFQ identical to CFQ in terms of hook
>> interfaces and io-context handling, and almost identical in terms
>> request-merging.
>>
>> 2) Enable people to test this first version bfq-mq. Code is purposely
>> overfull of log messages and invariant checks that halt the system on
>> failure (lock assertions, BUG_ONs, ...).
>>
>> To make it easier to revise commits, I'm sending the patches that
>> transform bfq into bfq-mq (last four patches in the branch [1]). They
>> work on two files, bfq-mq-iosched.c and bfq-mq.h, which, at the
>> beginning, are just copies of bfq-iosched.c and bfq.h.
>>
>
> Hi,
> this is just to inform that, as I just wrote to Bart, I have rebase
> the branch [1] against the current content of for-4.11/next.
>
> Jens, Omar, did you find the time to have a look at the main commits
> or to run some test?
I only looked at the core change you proposed for passing in the
bio as well, and Omar fixed up the icq exit part and I also applied
that patch. I haven't look at any of the bfq-mq patches at all yet.
Not sure what I can do with those, I don't think those are
particularly useful to anyone but you.
Might make more sense to post the conversion for review as a
whole.
--
Jens Axboe
^ permalink raw reply
* Re: [WIP PATCHSET 0/4] WIP branch for bfq-mq
From: Bart Van Assche @ 2017-02-13 22:38 UTC (permalink / raw)
To: paolo.valente@linaro.org
Cc: ulf.hansson@linaro.org, tj@kernel.org,
linux-kernel@vger.kernel.org, linux-block@vger.kernel.org,
broonie@kernel.org, linus.walleij@linaro.org, axboe@kernel.dk
In-Reply-To: <1487023626.2719.9.camel@sandisk.com>
On Mon, 2017-02-13 at 22:07 +0000, Bart Van Assche wrote:
> On Mon, 2017-02-13 at 22:07 +0100, Paolo Valente wrote:
> > but what do you think about trying this fix?
> =
> Sorry but with ... the same server I used for the previous test still
> didn't boot up properly. A screenshot is available at
> https://goo.gl/photos/Za9QVGCNe2BJBwxVA.
> =
> > Otherwise, if you have no news or suggestions, would you be willing to
> > try my micro-logging proposal https://github.com/Algodev-github/bfq-mq?
> =
> Sorry but it's not clear to me what logging mechanism you are referring
> to and how to enable it? Are you perhaps referring to
> CONFIG_BFQ_REDIRECT_TO_CONSOLE?
Anyway, a second screenshot has been added to the same album after I had
applied the following patch:
diff --git a/block/Makefile b/block/Makefile
index 1c04fe19e825..bf472ac82c08 100644
--- a/block/Makefile
+++ b/block/Makefile
@@ -2,6 +2,8 @@
# Makefile for the kernel block layer
#
=A0
+KBUILD_CFLAGS +=3D -DCONFIG_BFQ_REDIRECT_TO_CONSOLE
+
obj-$(CONFIG_BLOCK) :=3D bio.o elevator.o blk-core.o blk-tag.o blk-sysfs.o=
\
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0blk-f=
lush.o blk-settings.o blk-ioc.o blk-map.o \
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0blk-e=
xec.o blk-merge.o blk-softirq.o blk-timeout.o \
Bart.
Western Digital Corporation (and its subsidiaries) E-mail Confidentiality N=
otice & Disclaimer:
This e-mail and any files transmitted with it may contain confidential or l=
egally privileged information of WDC and/or its affiliates, and are intende=
d solely for the use of the individual or entity to which they are addresse=
d. If you are not the intended recipient, any disclosure, copying, distribu=
tion or any action taken or omitted to be taken in reliance on it, is prohi=
bited. If you have received this e-mail in error, please notify the sender =
immediately and delete the e-mail in its entirety from your system.
^ permalink raw reply related
* Re: [PATCH BUGFIX] block: make elevator_get robust against cross blk/blk-mq choice
From: Jens Axboe @ 2017-02-13 23:10 UTC (permalink / raw)
To: Omar Sandoval, Paolo Valente
Cc: Tejun Heo, linux-block, linux-kernel, ulf.hansson, linus.walleij,
broonie
In-Reply-To: <89b98d59-fcae-6b13-a6a1-6fe62967929d@kernel.dk>
On 02/13/2017 03:28 PM, Jens Axboe wrote:
> On 02/13/2017 03:09 PM, Omar Sandoval wrote:
>> On Mon, Feb 13, 2017 at 10:01:07PM +0100, Paolo Valente wrote:
>>> If, at boot, a legacy I/O scheduler is chosen for a device using blk-mq,
>>> or, viceversa, a blk-mq scheduler is chosen for a device using blk, then
>>> that scheduler is set and initialized without any check, driving the
>>> system into an inconsistent state. This commit addresses this issue by
>>> letting elevator_get fail for these wrong cross choices.
>>>
>>> Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
>>> ---
>>> block/elevator.c | 26 ++++++++++++++++++--------
>>> 1 file changed, 18 insertions(+), 8 deletions(-)
>>
>> Hey, Paolo,
>>
>> How exactly are you triggering this? In __elevator_change(), we do check
>> for mq or not mq:
>>
>> if (!e->uses_mq && q->mq_ops) {
>> elevator_put(e);
>> return -EINVAL;
>> }
>> if (e->uses_mq && !q->mq_ops) {
>> elevator_put(e);
>> return -EINVAL;
>> }
>>
>> We don't ever appear to call elevator_init() with a specific scheduler
>> name, and for the default we switch off of q->mq_ops and use the
>> defaults from Kconfig:
>>
>> if (q->mq_ops && q->nr_hw_queues == 1)
>> e = elevator_get(CONFIG_DEFAULT_SQ_IOSCHED, false);
>> else if (q->mq_ops)
>> e = elevator_get(CONFIG_DEFAULT_MQ_IOSCHED, false);
>> else
>> e = elevator_get(CONFIG_DEFAULT_IOSCHED, false);
>>
>> if (!e) {
>> printk(KERN_ERR
>> "Default I/O scheduler not found. " \
>> "Using noop/none.\n");
>> e = elevator_get("noop", false);
>> }
>>
>> So I guess this could happen if someone manually changed those Kconfig
>> options, but I don't see what other case would make this happen, could
>> you please explain?
>
> Was wondering the same - is it using the 'elevator=' boot parameter?
> Didn't look at that path just now, but that's the only one I could
> think of. If it is, I'd much prefer only using 'chosen_elevator' for
> the non-mq stuff, and the fix should be just that instead.
>
> So instead of:
>
> if (!e && *chosen_elevator) {
>
> do
>
> if (!e && !q->mq_ops && && *chosen_elevator) {
Confirmed, that's what it seems to be, and here's a real diff of the
above example that works for me:
diff --git a/block/elevator.c b/block/elevator.c
index 27ff1ed5a6fa..699d10f71a2c 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -207,11 +207,12 @@ int elevator_init(struct request_queue *q, char *name)
}
/*
- * Use the default elevator specified by config boot param or
- * config option. Don't try to load modules as we could be running
- * off async and request_module() isn't allowed from async.
+ * Use the default elevator specified by config boot param for
+ * non-mq devices, or by config option. Don't try to load modules
+ * as we could be running off async and request_module() isn't
+ * allowed from async.
*/
- if (!e && *chosen_elevator) {
+ if (!e && !q->mq_ops && *chosen_elevator) {
e = elevator_get(chosen_elevator, false);
if (!e)
printk(KERN_ERR "I/O scheduler %s not found\n",
--
Jens Axboe
^ permalink raw reply related
* Re: [PATCH v1 1/5] block: introduce bio_clone_bioset_partial()
From: Ming Lei @ 2017-02-14 1:04 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Shaohua Li, Jens Axboe, Linux Kernel Mailing List,
open list:SOFTWARE RAID (Multiple Disks) SUPPORT, linux-block,
NeilBrown
In-Reply-To: <20170213134654.GB22818@infradead.org>
On Mon, Feb 13, 2017 at 9:46 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, Feb 10, 2017 at 06:56:13PM +0800, Ming Lei wrote:
>> md still need bio clone(not the fast version) for behind write,
>> and it is more efficient to use bio_clone_bioset_partial().
>>
>> The idea is simple and just copy the bvecs range specified from
>> parameters.
>
> Given how few users bio_clone_bioset has I wonder if we shouldn't
> simply add the two new arguments to it instead of adding another
> indirection.
For md write-behind, looks we have to provide the two arguments,
could you explain a bit how we can do that by adding another indirection?
>
> Otherwise looks fine:
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Thanks!
--
Ming Lei
^ permalink raw reply
* [PATCH 4/7] nonblocking aio: return on congested block device
From: Goldwyn Rodrigues @ 2017-02-14 2:46 UTC (permalink / raw)
To: linux-fsdevel; +Cc: jack, Goldwyn Rodrigues, linux-block
In-Reply-To: <20170214024603.9563-1-rgoldwyn@suse.de>
From: Goldwyn Rodrigues <rgoldwyn@suse.com>
A new flag BIO_NONBLOCKING is introduced to identify bio's
orignating from iocb with IOCB_NONBLOCKING. struct request
are requested using BLK_MQ_REQ_NOWAIT if BIO_NONBLOCKING is set.
Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
block/blk-core.c | 13 +++++++++++--
block/blk-mq.c | 18 ++++++++++++++++--
fs/direct-io.c | 11 +++++++++--
include/linux/blk_types.h | 1 +
4 files changed, 37 insertions(+), 6 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index 14d7c07..9767573 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1257,6 +1257,11 @@ static struct request *get_request(struct request_queue *q, int op,
if (!IS_ERR(rq))
return rq;
+ if (bio_flagged(bio, BIO_NONBLOCKING)) {
+ blk_put_rl(rl);
+ return ERR_PTR(-EAGAIN);
+ }
+
if (!gfpflags_allow_blocking(gfp_mask) || unlikely(blk_queue_dying(q))) {
blk_put_rl(rl);
return rq;
@@ -2035,7 +2040,7 @@ blk_qc_t generic_make_request(struct bio *bio)
do {
struct request_queue *q = bdev_get_queue(bio->bi_bdev);
- if (likely(blk_queue_enter(q, false) == 0)) {
+ if (likely(blk_queue_enter(q, bio_flagged(bio, BIO_NONBLOCKING)) == 0)) {
ret = q->make_request_fn(q, bio);
blk_queue_exit(q);
@@ -2044,7 +2049,11 @@ blk_qc_t generic_make_request(struct bio *bio)
} else {
struct bio *bio_next = bio_list_pop(current->bio_list);
- bio_io_error(bio);
+ if (unlikely(bio_flagged(bio, BIO_NONBLOCKING))) {
+ bio->bi_error = -EAGAIN;
+ bio_endio(bio);
+ } else
+ bio_io_error(bio);
bio = bio_next;
}
} while (bio);
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 81caceb..7a7c674 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1213,6 +1213,8 @@ static struct request *blk_mq_map_request(struct request_queue *q,
trace_block_getrq(q, bio, op);
blk_mq_set_alloc_data(&alloc_data, q, 0, ctx, hctx);
+ if (bio_flagged(bio, BIO_NONBLOCKING))
+ alloc_data.flags |= BLK_MQ_REQ_NOWAIT;
rq = __blk_mq_alloc_request(&alloc_data, op, op_flags);
data->hctx = alloc_data.hctx;
@@ -1286,8 +1288,14 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
return BLK_QC_T_NONE;
rq = blk_mq_map_request(q, bio, &data);
- if (unlikely(!rq))
+ if (unlikely(!rq)) {
+ if (bio_flagged(bio, BIO_NONBLOCKING))
+ bio->bi_error = -EAGAIN;
+ else
+ bio->bi_error = -EIO;
+ bio_endio(bio);
return BLK_QC_T_NONE;
+ }
cookie = blk_tag_to_qc_t(rq->tag, data.hctx->queue_num);
@@ -1381,8 +1389,14 @@ static blk_qc_t blk_sq_make_request(struct request_queue *q, struct bio *bio)
request_count = blk_plug_queued_count(q);
rq = blk_mq_map_request(q, bio, &data);
- if (unlikely(!rq))
+ if (unlikely(!rq)) {
+ if (bio_flagged(bio, BIO_NONBLOCKING))
+ bio->bi_error = -EAGAIN;
+ else
+ bio->bi_error = -EIO;
+ bio_endio(bio);
return BLK_QC_T_NONE;
+ }
cookie = blk_tag_to_qc_t(rq->tag, data.hctx->queue_num);
diff --git a/fs/direct-io.c b/fs/direct-io.c
index fb9aa16..9997fed 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -386,6 +386,9 @@ dio_bio_alloc(struct dio *dio, struct dio_submit *sdio,
else
bio->bi_end_io = dio_bio_end_io;
+ if (dio->iocb->ki_flags & IOCB_NONBLOCKING)
+ bio_set_flag(bio, BIO_NONBLOCKING);
+
sdio->bio = bio;
sdio->logical_offset_in_bio = sdio->cur_page_fs_offset;
}
@@ -480,8 +483,12 @@ static int dio_bio_complete(struct dio *dio, struct bio *bio)
unsigned i;
int err;
- if (bio->bi_error)
- dio->io_error = -EIO;
+ if (bio->bi_error) {
+ if (bio_flagged(bio, BIO_NONBLOCKING))
+ dio->io_error = bio->bi_error;
+ else
+ dio->io_error = -EIO;
+ }
if (dio->is_async && dio->op == REQ_OP_READ && dio->should_dirty) {
err = bio->bi_error;
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index cd395ec..94855cf 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -119,6 +119,7 @@ struct bio {
#define BIO_QUIET 6 /* Make BIO Quiet */
#define BIO_CHAIN 7 /* chained bio, ->bi_remaining in effect */
#define BIO_REFFED 8 /* bio has elevated ->bi_cnt */
+#define BIO_NONBLOCKING 9 /* don't block over blk device congestion */
/*
* Flags starting here get preserved by bio_reset() - this includes
--
2.10.2
^ permalink raw reply related
* Re: [PATCH 4/7] nonblocking aio: return on congested block device
From: Ming Lei @ 2017-02-14 3:55 UTC (permalink / raw)
To: Goldwyn Rodrigues
Cc: Linux FS Devel, Jan Kara, Goldwyn Rodrigues, linux-block
In-Reply-To: <20170214024603.9563-5-rgoldwyn@suse.de>
On Tue, Feb 14, 2017 at 10:46 AM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>
> A new flag BIO_NONBLOCKING is introduced to identify bio's
> orignating from iocb with IOCB_NONBLOCKING. struct request
> are requested using BLK_MQ_REQ_NOWAIT if BIO_NONBLOCKING is set.
>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
> block/blk-core.c | 13 +++++++++++--
> block/blk-mq.c | 18 ++++++++++++++++--
> fs/direct-io.c | 11 +++++++++--
> include/linux/blk_types.h | 1 +
> 4 files changed, 37 insertions(+), 6 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 14d7c07..9767573 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1257,6 +1257,11 @@ static struct request *get_request(struct request_queue *q, int op,
> if (!IS_ERR(rq))
> return rq;
>
> + if (bio_flagged(bio, BIO_NONBLOCKING)) {
> + blk_put_rl(rl);
> + return ERR_PTR(-EAGAIN);
> + }
> +
> if (!gfpflags_allow_blocking(gfp_mask) || unlikely(blk_queue_dying(q))) {
> blk_put_rl(rl);
> return rq;
> @@ -2035,7 +2040,7 @@ blk_qc_t generic_make_request(struct bio *bio)
> do {
> struct request_queue *q = bdev_get_queue(bio->bi_bdev);
>
> - if (likely(blk_queue_enter(q, false) == 0)) {
> + if (likely(blk_queue_enter(q, bio_flagged(bio, BIO_NONBLOCKING)) == 0)) {
> ret = q->make_request_fn(q, bio);
>
> blk_queue_exit(q);
> @@ -2044,7 +2049,11 @@ blk_qc_t generic_make_request(struct bio *bio)
> } else {
> struct bio *bio_next = bio_list_pop(current->bio_list);
>
> - bio_io_error(bio);
> + if (unlikely(bio_flagged(bio, BIO_NONBLOCKING))) {
> + bio->bi_error = -EAGAIN;
> + bio_endio(bio);
> + } else
> + bio_io_error(bio);
> bio = bio_next;
> }
> } while (bio);
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 81caceb..7a7c674 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1213,6 +1213,8 @@ static struct request *blk_mq_map_request(struct request_queue *q,
>
> trace_block_getrq(q, bio, op);
> blk_mq_set_alloc_data(&alloc_data, q, 0, ctx, hctx);
> + if (bio_flagged(bio, BIO_NONBLOCKING))
> + alloc_data.flags |= BLK_MQ_REQ_NOWAIT;
> rq = __blk_mq_alloc_request(&alloc_data, op, op_flags);
>
> data->hctx = alloc_data.hctx;
> @@ -1286,8 +1288,14 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
> return BLK_QC_T_NONE;
>
> rq = blk_mq_map_request(q, bio, &data);
> - if (unlikely(!rq))
> + if (unlikely(!rq)) {
> + if (bio_flagged(bio, BIO_NONBLOCKING))
> + bio->bi_error = -EAGAIN;
> + else
> + bio->bi_error = -EIO;
> + bio_endio(bio);
> return BLK_QC_T_NONE;
> + }
>
> cookie = blk_tag_to_qc_t(rq->tag, data.hctx->queue_num);
>
> @@ -1381,8 +1389,14 @@ static blk_qc_t blk_sq_make_request(struct request_queue *q, struct bio *bio)
> request_count = blk_plug_queued_count(q);
>
> rq = blk_mq_map_request(q, bio, &data);
> - if (unlikely(!rq))
> + if (unlikely(!rq)) {
> + if (bio_flagged(bio, BIO_NONBLOCKING))
> + bio->bi_error = -EAGAIN;
> + else
> + bio->bi_error = -EIO;
> + bio_endio(bio);
> return BLK_QC_T_NONE;
> + }
There are other places in which blocking may be triggered, such
as allocating for bio clone, wbt_wait(), and sleep in .make_request(),
like md, dm and bcache's.
IMO it should be hard to deal with all, so what is the expection for
flag of BIO_NONBLOCKING?
>
> cookie = blk_tag_to_qc_t(rq->tag, data.hctx->queue_num);
>
> diff --git a/fs/direct-io.c b/fs/direct-io.c
> index fb9aa16..9997fed 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -386,6 +386,9 @@ dio_bio_alloc(struct dio *dio, struct dio_submit *sdio,
> else
> bio->bi_end_io = dio_bio_end_io;
>
> + if (dio->iocb->ki_flags & IOCB_NONBLOCKING)
> + bio_set_flag(bio, BIO_NONBLOCKING);
> +
> sdio->bio = bio;
> sdio->logical_offset_in_bio = sdio->cur_page_fs_offset;
> }
> @@ -480,8 +483,12 @@ static int dio_bio_complete(struct dio *dio, struct bio *bio)
> unsigned i;
> int err;
>
> - if (bio->bi_error)
> - dio->io_error = -EIO;
> + if (bio->bi_error) {
> + if (bio_flagged(bio, BIO_NONBLOCKING))
> + dio->io_error = bio->bi_error;
> + else
> + dio->io_error = -EIO;
> + }
>
> if (dio->is_async && dio->op == REQ_OP_READ && dio->should_dirty) {
> err = bio->bi_error;
> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> index cd395ec..94855cf 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -119,6 +119,7 @@ struct bio {
> #define BIO_QUIET 6 /* Make BIO Quiet */
> #define BIO_CHAIN 7 /* chained bio, ->bi_remaining in effect */
> #define BIO_REFFED 8 /* bio has elevated ->bi_cnt */
> +#define BIO_NONBLOCKING 9 /* don't block over blk device congestion */
>
> /*
> * Flags starting here get preserved by bio_reset() - this includes
> --
> 2.10.2
>
--
Ming Lei
^ permalink raw reply
* Re: [PATCH BUGFIX] block: make elevator_get robust against cross blk/blk-mq choice
From: Hannes Reinecke @ 2017-02-14 6:58 UTC (permalink / raw)
To: Jens Axboe, Omar Sandoval, Paolo Valente
Cc: Tejun Heo, linux-block, linux-kernel, ulf.hansson, linus.walleij,
broonie
In-Reply-To: <89b98d59-fcae-6b13-a6a1-6fe62967929d@kernel.dk>
On 02/13/2017 11:28 PM, Jens Axboe wrote:
> On 02/13/2017 03:09 PM, Omar Sandoval wrote:
>> On Mon, Feb 13, 2017 at 10:01:07PM +0100, Paolo Valente wrote:
>>> If, at boot, a legacy I/O scheduler is chosen for a device using blk-mq,
>>> or, viceversa, a blk-mq scheduler is chosen for a device using blk, then
>>> that scheduler is set and initialized without any check, driving the
>>> system into an inconsistent state. This commit addresses this issue by
>>> letting elevator_get fail for these wrong cross choices.
>>>
>>> Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
>>> ---
>>> block/elevator.c | 26 ++++++++++++++++++--------
>>> 1 file changed, 18 insertions(+), 8 deletions(-)
>>
>> Hey, Paolo,
>>
>> How exactly are you triggering this? In __elevator_change(), we do check
>> for mq or not mq:
>>
>> if (!e->uses_mq && q->mq_ops) {
>> elevator_put(e);
>> return -EINVAL;
>> }
>> if (e->uses_mq && !q->mq_ops) {
>> elevator_put(e);
>> return -EINVAL;
>> }
>>
>> We don't ever appear to call elevator_init() with a specific scheduler
>> name, and for the default we switch off of q->mq_ops and use the
>> defaults from Kconfig:
>>
>> if (q->mq_ops && q->nr_hw_queues == 1)
>> e = elevator_get(CONFIG_DEFAULT_SQ_IOSCHED, false);
>> else if (q->mq_ops)
>> e = elevator_get(CONFIG_DEFAULT_MQ_IOSCHED, false);
>> else
>> e = elevator_get(CONFIG_DEFAULT_IOSCHED, false);
>>
>> if (!e) {
>> printk(KERN_ERR
>> "Default I/O scheduler not found. " \
>> "Using noop/none.\n");
>> e = elevator_get("noop", false);
>> }
>>
>> So I guess this could happen if someone manually changed those Kconfig
>> options, but I don't see what other case would make this happen, could
>> you please explain?
>
> Was wondering the same - is it using the 'elevator=' boot parameter?
> Didn't look at that path just now, but that's the only one I could
> think of. If it is, I'd much prefer only using 'chosen_elevator' for
> the non-mq stuff, and the fix should be just that instead.
>
[ .. ]
While we're at the topic:
Can't we use the same names for legacy and mq scheduler?
It's quite an unnecessary complication to have
'noop', 'deadline', and 'cfq' for legacy, but 'none' and 'mq-deadline'
for mq. If we could use 'noop' and 'deadline' for mq, too, the existing
settings or udev rules will continue to work and we wouldn't get any
annoying and pointless warnings here...
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: F. Imend�rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N�rnberg)
^ permalink raw reply
* Re: [PATCH BUGFIX] block: make elevator_get robust against cross blk/blk-mq choice
From: Omar Sandoval @ 2017-02-14 7:07 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Jens Axboe, Paolo Valente, Tejun Heo, linux-block, linux-kernel,
ulf.hansson, linus.walleij, broonie
In-Reply-To: <d6e69d7b-dc69-90ec-0130-dc774fd4da2a@suse.de>
On Tue, Feb 14, 2017 at 07:58:22AM +0100, Hannes Reinecke wrote:
> While we're at the topic:
>
> Can't we use the same names for legacy and mq scheduler?
> It's quite an unnecessary complication to have
> 'noop', 'deadline', and 'cfq' for legacy, but 'none' and 'mq-deadline'
> for mq. If we could use 'noop' and 'deadline' for mq, too, the existing
> settings or udev rules will continue to work and we wouldn't get any
> annoying and pointless warnings here...
I mentioned this to Jens a little while ago but I didn't feel strongly
enough to push the issue. I also like this idea -- it makes the
transition to blk-mq a little more transparent.
^ permalink raw reply
* Re: [PATCH BUGFIX] block: make elevator_get robust against cross blk/blk-mq choice
From: Hannes Reinecke @ 2017-02-14 7:11 UTC (permalink / raw)
To: Omar Sandoval
Cc: Jens Axboe, Paolo Valente, Tejun Heo, linux-block, linux-kernel,
ulf.hansson, linus.walleij, broonie
In-Reply-To: <20170214070727.GA19880@vader>
On 02/14/2017 08:07 AM, Omar Sandoval wrote:
> On Tue, Feb 14, 2017 at 07:58:22AM +0100, Hannes Reinecke wrote:
>> While we're at the topic:
>>
>> Can't we use the same names for legacy and mq scheduler?
>> It's quite an unnecessary complication to have
>> 'noop', 'deadline', and 'cfq' for legacy, but 'none' and 'mq-deadline'
>> for mq. If we could use 'noop' and 'deadline' for mq, too, the existing
>> settings or udev rules will continue to work and we wouldn't get any
>> annoying and pointless warnings here...
>
> I mentioned this to Jens a little while ago but I didn't feel strongly
> enough to push the issue. I also like this idea -- it makes the
> transition to blk-mq a little more transparent.
>
And saves us _a lot_ of support cases :-)
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: F. Imend�rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N�rnberg)
^ permalink raw reply
* [PATCH] sd: Check for unaligned partial completion
From: Damien Le Moal @ 2017-02-14 8:10 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Shaun Tancheff, Damien Le Moal
Commit "mpt3sas: Force request partial completion alignment" was not
considering the case of REQ_TYPE_FS commands not operating on sector
size units (e.g. REQ_OP_ZONE_REPORT and its 64B aligned partial
replies). This could result is incorrectly retrying (forever) those
commands.
Move the partial completion alignement check of mpt3sas to sd_done so
that the check comes after good_bytes & resid corrections done in that
function depending on the request command.
Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
drivers/scsi/mpt3sas/mpt3sas_scsih.c | 15 ---------------
drivers/scsi/sd.c | 20 ++++++++++++++++++++
2 files changed, 20 insertions(+), 15 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 0b5b423..1961535 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -4658,7 +4658,6 @@ _scsih_io_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, u8 msix_index, u32 reply)
struct MPT3SAS_DEVICE *sas_device_priv_data;
u32 response_code = 0;
unsigned long flags;
- unsigned int sector_sz;
mpi_reply = mpt3sas_base_get_reply_virt_addr(ioc, reply);
scmd = _scsih_scsi_lookup_get_clear(ioc, smid);
@@ -4717,20 +4716,6 @@ _scsih_io_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, u8 msix_index, u32 reply)
}
xfer_cnt = le32_to_cpu(mpi_reply->TransferCount);
-
- /* In case of bogus fw or device, we could end up having
- * unaligned partial completion. We can force alignment here,
- * then scsi-ml does not need to handle this misbehavior.
- */
- sector_sz = scmd->device->sector_size;
- if (unlikely(scmd->request->cmd_type == REQ_TYPE_FS && sector_sz &&
- xfer_cnt % sector_sz)) {
- sdev_printk(KERN_INFO, scmd->device,
- "unaligned partial completion avoided (xfer_cnt=%u, sector_sz=%u)\n",
- xfer_cnt, sector_sz);
- xfer_cnt = round_down(xfer_cnt, sector_sz);
- }
-
scsi_set_resid(scmd, scsi_bufflen(scmd) - xfer_cnt);
if (ioc_status & MPI2_IOCSTATUS_FLAG_LOG_INFO_AVAILABLE)
log_info = le32_to_cpu(mpi_reply->IOCLogInfo);
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 1f5d92a..a3d8bc4 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1790,6 +1790,8 @@ static int sd_done(struct scsi_cmnd *SCpnt)
{
int result = SCpnt->result;
unsigned int good_bytes = result ? 0 : scsi_bufflen(SCpnt);
+ unsigned int sector_sz = SCpnt->device->sector_size;
+ unsigned int resid;
struct scsi_sense_hdr sshdr;
struct scsi_disk *sdkp = scsi_disk(SCpnt->request->rq_disk);
struct request *req = SCpnt->request;
@@ -1829,6 +1831,24 @@ static int sd_done(struct scsi_cmnd *SCpnt)
}
sdkp->medium_access_timed_out = 0;
+ /*
+ * In case of bogus fw or device, we could end up having
+ * unaligned partial completion. Check this here.
+ */
+ resid = scsi_get_resid(SCpnt);
+ if (SCpnt->request->cmd_type == REQ_TYPE_FS &&
+ resid & (sector_sz - 1)) {
+ SCSI_LOG_HLCOMPLETE(1, scmd_printk(KERN_INFO, SCpnt,
+ "Unaligned partial completion (resid=%u, sector_sz=%u)\n",
+ resid, sector_sz));
+ resid = round_up(resid, sector_sz);
+ if (resid < good_bytes)
+ good_bytes -= resid;
+ else
+ good_bytes = 0;
+ scsi_set_resid(SCpnt, resid);
+ }
+
if (driver_byte(result) != DRIVER_SENSE &&
(!sense_valid || sense_deferred))
goto out;
--
2.9.3
Western Digital Corporation (and its subsidiaries) E-mail Confidentiality Notice & Disclaimer:
This e-mail and any files transmitted with it may contain confidential or legally privileged information of WDC and/or its affiliates, and are intended solely for the use of the individual or entity to which they are addressed. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited. If you have received this e-mail in error, please notify the sender immediately and delete the e-mail in its entirety from your system.
^ permalink raw reply related
* Re: [PATCH BUGFIX] block: make elevator_get robust against cross blk/blk-mq choice
From: Paolo Valente @ 2017-02-14 8:14 UTC (permalink / raw)
To: Jens Axboe
Cc: Omar Sandoval, Tejun Heo, linux-block, Linux-Kernal, Ulf Hansson,
Linus Walleij, broonie
In-Reply-To: <b1a1ad6d-6e22-7d55-bbad-40807d95f59b@kernel.dk>
> Il giorno 14 feb 2017, alle ore 00:10, Jens Axboe <axboe@kernel.dk> ha =
scritto:
>=20
> On 02/13/2017 03:28 PM, Jens Axboe wrote:
>> On 02/13/2017 03:09 PM, Omar Sandoval wrote:
>>> On Mon, Feb 13, 2017 at 10:01:07PM +0100, Paolo Valente wrote:
>>>> If, at boot, a legacy I/O scheduler is chosen for a device using =
blk-mq,
>>>> or, viceversa, a blk-mq scheduler is chosen for a device using blk, =
then
>>>> that scheduler is set and initialized without any check, driving =
the
>>>> system into an inconsistent state. This commit addresses this issue =
by
>>>> letting elevator_get fail for these wrong cross choices.
>>>>=20
>>>> Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
>>>> ---
>>>> block/elevator.c | 26 ++++++++++++++++++--------
>>>> 1 file changed, 18 insertions(+), 8 deletions(-)
>>>=20
>>> Hey, Paolo,
>>>=20
>>> How exactly are you triggering this? In __elevator_change(), we do =
check
>>> for mq or not mq:
>>>=20
>>> if (!e->uses_mq && q->mq_ops) {
>>> elevator_put(e);
>>> return -EINVAL;
>>> }
>>> if (e->uses_mq && !q->mq_ops) {
>>> elevator_put(e);
>>> return -EINVAL;
>>> }
>>>=20
>>> We don't ever appear to call elevator_init() with a specific =
scheduler
>>> name, and for the default we switch off of q->mq_ops and use the
>>> defaults from Kconfig:
>>>=20
>>> if (q->mq_ops && q->nr_hw_queues =3D=3D 1)
>>> e =3D elevator_get(CONFIG_DEFAULT_SQ_IOSCHED, false);
>>> else if (q->mq_ops)
>>> e =3D elevator_get(CONFIG_DEFAULT_MQ_IOSCHED, false);
>>> else
>>> e =3D elevator_get(CONFIG_DEFAULT_IOSCHED, false);
>>>=20
>>> if (!e) {
>>> printk(KERN_ERR
>>> "Default I/O scheduler not found. " \
>>> "Using noop/none.\n");
>>> e =3D elevator_get("noop", false);
>>> }
>>>=20
>>> So I guess this could happen if someone manually changed those =
Kconfig
>>> options, but I don't see what other case would make this happen, =
could
>>> you please explain?
>>=20
>> Was wondering the same - is it using the 'elevator=3D' boot =
parameter?
>> Didn't look at that path just now, but that's the only one I could
>> think of. If it is, I'd much prefer only using 'chosen_elevator' for
>> the non-mq stuff, and the fix should be just that instead.
>>=20
>> So instead of:
>>=20
>> if (!e && *chosen_elevator) {
>>=20
>> do
>>=20
>> if (!e && !q->mq_ops && && *chosen_elevator) {
>=20
> Confirmed, that's what it seems to be, and here's a real diff of the
> above example that works for me:
>=20
> diff --git a/block/elevator.c b/block/elevator.c
> index 27ff1ed5a6fa..699d10f71a2c 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -207,11 +207,12 @@ int elevator_init(struct request_queue *q, char =
*name)
> }
>=20
> /*
> - * Use the default elevator specified by config boot param or
> - * config option. Don't try to load modules as we could be =
running
> - * off async and request_module() isn't allowed from async.
> + * Use the default elevator specified by config boot param for
> + * non-mq devices, or by config option.
I don't fully get this choice: being able to change the default I/O
scheduler through the command line has been rather useful for me,
saving me a lot of recompilations, and such a feature seems widespread
among (at least power) users. However, mine is of course just an
opinion, and I may be missing the main point also in this case.
Thanks,
Paolo
> Don't try to load modules
> + * as we could be running off async and request_module() isn't
> + * allowed from async.
> */
> - if (!e && *chosen_elevator) {
> + if (!e && !q->mq_ops && *chosen_elevator) {
> e =3D elevator_get(chosen_elevator, false);
> if (!e)
> printk(KERN_ERR "I/O scheduler %s not found\n",
>=20
> --=20
> Jens Axboe
^ permalink raw reply
* Re: [PATCH V5 1/4] block: sed-opal: change ioctl to take user pointer instead of unsinged long
From: Christoph Hellwig @ 2017-02-14 8:17 UTC (permalink / raw)
To: Scott Bauer
Cc: linux-nvme, David.Laight, arnd, axboe, keith.busch,
jonathan.derrick, hch, linux-kernel, linux-block
In-Reply-To: <20170213161509.GA18913@sbauer-Z170X-UD5>
On Mon, Feb 13, 2017 at 09:15:10AM -0700, Scott Bauer wrote:
> esOn Mon, Feb 13, 2017 at 09:11:09AM -0700, Scott Bauer wrote:
> > Signed-off-by: Scott Bauer <scott.bauer@intel.com>
> > ---
> > block/sed-opal.c | 6 ++++--
> > drivers/nvme/host/core.c | 3 ++-
> > include/linux/sed-opal.h | 4 ++--
> > 3 files changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/block/sed-opal.c b/block/sed-opal.c
> > index bf1406e..2448d4a 100644
> > --- a/block/sed-opal.c
> > +++ b/block/sed-opal.c
> > @@ -2344,9 +2344,11 @@ bool opal_unlock_from_suspend(struct opal_dev *dev)
> > }
> > EXPORT_SYMBOL(opal_unlock_from_suspend);
> >
> > -int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
> > +int sed_ioctl(struct opal_dev *dev, unsigned int cmd, void __user *arg)
> > {
> > - void __user *arg = (void __user *)ptr;
> > + void *ioctl_ptr;
> > + int ret = -ENOTTY;
> > + unsigned int cmd_size = _IOC_SIZE(cmd);
>
> ugh, I apparently messed up my rebase these should be in patch 2 or maybe I should
> sqash p1 and p2 together.
I guess it should be 1 and 3. No real opinipon on that, but can you
chose a simpler and more fitting name than ioctl_ptr? I'd suggest 'p'.
^ permalink raw reply
* Re: [PATCH V5 2/4] uapi: sed-opal fix IOW for activate lsp to use correct struct
From: Christoph Hellwig @ 2017-02-14 8:18 UTC (permalink / raw)
To: Scott Bauer
Cc: linux-nvme, David.Laight, arnd, axboe, keith.busch,
jonathan.derrick, hch, linux-kernel, linux-block
In-Reply-To: <1487002272-17940-3-git-send-email-scott.bauer@intel.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Let's get this one in ASAP while waiting for a respin of the KASAN
fix.
^ permalink raw reply
* Re: [PATCH V5 4/4] Maintainers: Modify SED list from nvme to block
From: Christoph Hellwig @ 2017-02-14 8:18 UTC (permalink / raw)
To: Scott Bauer
Cc: keith.busch, arnd, hch, linux-kernel, linux-nvme, axboe,
David.Laight, linux-block, jonathan.derrick
In-Reply-To: <1487002272-17940-5-git-send-email-scott.bauer@intel.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Let's get it in ASAP as well.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply
* Re: [PATCH] sd: Check for unaligned partial completion
From: Christoph Hellwig @ 2017-02-14 8:23 UTC (permalink / raw)
To: Damien Le Moal; +Cc: Jens Axboe, linux-block, Shaun Tancheff
In-Reply-To: <20170214081030.24558-1-damien.lemoal@wdc.com>
> + unsigned int sector_sz = SCpnt->device->sector_size;
Can you spell out size?
> + /*
> + * In case of bogus fw or device, we could end up having
> + * unaligned partial completion. Check this here.
> + */
> + resid = scsi_get_resid(SCpnt);
> + if (SCpnt->request->cmd_type == REQ_TYPE_FS &&
> + resid & (sector_sz - 1)) {
->done is only called for fs requests, so you can remove this check.
^ 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