* Re: [lkp-robot] [scsi, block] 0dba1314d4: WARNING:at_fs/sysfs/dir.c:#sysfs_warn_dup
From: James Bottomley @ 2017-02-09 0:08 UTC (permalink / raw)
To: Dan Williams, Jens Axboe
Cc: Christoph Hellwig, kernel test robot, Bart Van Assche,
Martin K. Petersen, Jan Kara, Omar Sandoval, Omar Sandoval, LKML,
Jens Axboe, LKP, linux-scsi, linux-block
In-Reply-To: <CAPcyv4jkak-gX3gQaBJiEk1G79skdN2F9WL-sK2DjPwFhA8rdg@mail.gmail.com>
On Mon, 2017-02-06 at 21:42 -0800, Dan Williams wrote:
> On Mon, Feb 6, 2017 at 8:09 PM, Jens Axboe <axboe@fb.com> wrote:
> > On 02/06/2017 05:14 PM, James Bottomley wrote:
> > > On Sun, 2017-02-05 at 21:13 -0800, Dan Williams wrote:
> > > > On Sun, Feb 5, 2017 at 1:13 AM, Christoph Hellwig <hch@lst.de>
> > > > wrote:
> > > > > Dan,
> > > > >
> > > > > can you please quote your emails? I can't find any content
> > > > > inbetween all these quotes.
> > > >
> > > > Sorry, I'm using gmail, but I'll switch to attaching the logs.
> > > >
> > > > So with help from Xiaolong I was able to reproduce this, and it
> > > > does
> > > > not appear to be a regression. We simply change the failure
> > > > output of
> > > > an existing bug. Attached is a log of the same test on v4.10
> > > > -rc7
> > > > (i.e. without the recent block/scsi fixes), and it shows sda
> > > > being
> > > > registered twice.
> > > >
> > > > "[ 6.647077] kobject (d5078ca4): tried to init an
> > > > initialized
> > > > object, something is seriously wrong."
> > > >
> > > > The change that "scsi, block: fix duplicate bdi name
> > > > registration
> > > > crashes" makes is to properly try to register sdb since the sda
> > > > devt
> > > > is still alive. However that's not a fix because we've managed
> > > > to
> > > > call blk_register_queue() twice on the same queue.
> > >
> > > OK, time to involve others: linux-scsi and linux-block cc'd and
> > > I've
> > > inserted the log below.
> > >
> > > James
> > >
> > > ---
> > >
> > > [ 5.969672] scsi host0: scsi_debug: version 1.86 [20160430]
> > > [ 5.969672] dev_size_mb=8, opts=0x0, submit_queues=1,
> > > statistics=0
> > > [ 5.971895] scsi 0:0:0:0: Direct-Access Linux
> > > scsi_debug 0186 PQ: 0 ANSI: 7
> > > [ 6.006983] sd 0:0:0:0: [sda] 16384 512-byte logical blocks:
> > > (8.39 MB/8.00 MiB)
> > > [ 6.026965] sd 0:0:0:0: [sda] Write Protect is off
> > > [ 6.027870] sd 0:0:0:0: [sda] Mode Sense: 73 00 10 08
> > > [ 6.066962] sd 0:0:0:0: [sda] Write cache: enabled, read
> > > cache: enabled, supports DPO and FUA
> > > [ 6.486962] sd 0:0:0:0: [sda] Attached SCSI disk
> > > [ 6.488377] sd 0:0:0:0: [sda] Synchronizing SCSI cache
> > > [ 6.489455] sd 0:0:0:0: Attached scsi generic sg0 type 0
> > > [ 6.526982] sd 0:0:0:0: [sda] 16384 512-byte logical blocks:
> > > (8.39 MB/8.00 MiB)
> > > [ 6.546964] sd 0:0:0:0: [sda] Write Protect is off
> > > [ 6.547873] sd 0:0:0:0: [sda] Mode Sense: 73 00 10 08
> > > [ 6.586963] sd 0:0:0:0: [sda] Write cache: enabled, read
> > > cache: enabled, supports DPO and FUA
> > > [ 6.647077] kobject (d5078ca4): tried to init an initialized
> > > object, something is seriously wrong.
> >
> > So sda is probed twice, and hilarity ensues when we try to register
> > it
> > twice. I can't reproduce this, using scsi_debug and with
> > scsi_async
> > enabled.
> >
> > This is running linux-next? What's your .config?
> >
>
> The original failure report is here:
>
> http://marc.info/?l=linux-kernel&m=148619222300774&w=2
>
> ...but it reproduces on current mainline with the same config. I
> haven't spotted what makes scsi_debug behave like this.
Looking at the config, it's a static debug with report luns enabled.
Is it as simple as the fact that we probe lun 0 manually to see if the
target exists, but then we don't account for the fact that we already
did this, so if it turns up again in the report lun scan, we'll probe
it again leading to a double add. If that theory is correct, this may
be the fix (compile tested only).
James
---
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 6f7128f..ba4be08 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1441,6 +1441,10 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
for (lunp = &lun_data[1]; lunp <= &lun_data[num_luns]; lunp++) {
lun = scsilun_to_int(lunp);
+ if (lun == 0)
+ /* already scanned LUN 0 */
+ continue;
+
if (lun > sdev->host->max_lun) {
sdev_printk(KERN_WARNING, sdev,
"lun%llu has a LUN larger than"
^ permalink raw reply related
* [PATCH v2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN
From: Scott Bauer @ 2017-02-08 23:00 UTC (permalink / raw)
To: linux-nvme
Cc: keith.busch, hch, linux-kernel, axboe, linux-block, Scott Bauer,
jonathan.derrick
When CONFIG_KASAN is enabled, compilation fails:
block/sed-opal.c: In function 'sed_ioctl':
block/sed-opal.c:2447:1: error: the frame size of 2256 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
Moved all the ioctl structures off the stack to a static
union to prevent oversized stack frame size.
Fixes: 455a7b238cd6 ("block: Add Sed-opal library")
Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Scott Bauer <scott.bauer@intel.com>
---
block/sed-opal.c | 187 ++++++++++++++++++++++++++++++-------------------------
1 file changed, 102 insertions(+), 85 deletions(-)
diff --git a/block/sed-opal.c b/block/sed-opal.c
index bf1406e..7979d12 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -2344,8 +2344,21 @@ bool opal_unlock_from_suspend(struct opal_dev *dev)
}
EXPORT_SYMBOL(opal_unlock_from_suspend);
+static union {
+ struct opal_lock_unlock lk_unlk;
+ struct opal_key opal_key;
+ struct opal_lr_act opal_lr_act;
+ struct opal_new_pw opal_pw;
+ struct opal_session_info session;
+ struct opal_user_lr_setup lrs;
+ struct opal_mbr_data mbr;
+} u;
+
+DEFINE_MUTEX(ioctl_lock);
+
int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
{
+ int ret = -ENOTTY;
void __user *arg = (void __user *)ptr;
if (!capable(CAP_SYS_ADMIN))
@@ -2355,94 +2368,98 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
return -ENOTSUPP;
}
+ mutex_lock(&ioctl_lock);
switch (cmd) {
- case IOC_OPAL_SAVE: {
- struct opal_lock_unlock lk_unlk;
-
- if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
- return -EFAULT;
- return opal_save(dev, &lk_unlk);
- }
- case IOC_OPAL_LOCK_UNLOCK: {
- struct opal_lock_unlock lk_unlk;
-
- if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
- return -EFAULT;
- return opal_lock_unlock(dev, &lk_unlk);
- }
- case IOC_OPAL_TAKE_OWNERSHIP: {
- struct opal_key opal_key;
-
- if (copy_from_user(&opal_key, arg, sizeof(opal_key)))
- return -EFAULT;
- return opal_take_ownership(dev, &opal_key);
- }
- case IOC_OPAL_ACTIVATE_LSP: {
- struct opal_lr_act opal_lr_act;
-
- if (copy_from_user(&opal_lr_act, arg, sizeof(opal_lr_act)))
- return -EFAULT;
- return opal_activate_lsp(dev, &opal_lr_act);
- }
- case IOC_OPAL_SET_PW: {
- struct opal_new_pw opal_pw;
-
- if (copy_from_user(&opal_pw, arg, sizeof(opal_pw)))
- return -EFAULT;
- return opal_set_new_pw(dev, &opal_pw);
- }
- case IOC_OPAL_ACTIVATE_USR: {
- struct opal_session_info session;
-
- if (copy_from_user(&session, arg, sizeof(session)))
- return -EFAULT;
- return opal_activate_user(dev, &session);
- }
- case IOC_OPAL_REVERT_TPR: {
- struct opal_key opal_key;
-
- if (copy_from_user(&opal_key, arg, sizeof(opal_key)))
- return -EFAULT;
- return opal_reverttper(dev, &opal_key);
- }
- case IOC_OPAL_LR_SETUP: {
- struct opal_user_lr_setup lrs;
-
- if (copy_from_user(&lrs, arg, sizeof(lrs)))
- return -EFAULT;
- return opal_setup_locking_range(dev, &lrs);
- }
- case IOC_OPAL_ADD_USR_TO_LR: {
- struct opal_lock_unlock lk_unlk;
-
- if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
- return -EFAULT;
- return opal_add_user_to_lr(dev, &lk_unlk);
- }
- case IOC_OPAL_ENABLE_DISABLE_MBR: {
- struct opal_mbr_data mbr;
-
- if (copy_from_user(&mbr, arg, sizeof(mbr)))
- return -EFAULT;
- return opal_enable_disable_shadow_mbr(dev, &mbr);
- }
- case IOC_OPAL_ERASE_LR: {
- struct opal_session_info session;
-
- if (copy_from_user(&session, arg, sizeof(session)))
- return -EFAULT;
- return opal_erase_locking_range(dev, &session);
- }
- case IOC_OPAL_SECURE_ERASE_LR: {
- struct opal_session_info session;
-
- if (copy_from_user(&session, arg, sizeof(session)))
- return -EFAULT;
- return opal_secure_erase_locking_range(dev, &session);
- }
+ case IOC_OPAL_SAVE:
+ if (copy_from_user(&u.lk_unlk, arg, sizeof(u.lk_unlk))) {
+ ret = -EFAULT;
+ goto out;
+ }
+ ret = opal_save(dev, &u.lk_unlk);
+ break;
+ case IOC_OPAL_LOCK_UNLOCK:
+ if (copy_from_user(&u.lk_unlk, arg, sizeof(u.lk_unlk))) {
+ ret = -EFAULT;
+ goto out;
+ }
+ ret = opal_lock_unlock(dev, &u.lk_unlk);
+ break;
+ case IOC_OPAL_TAKE_OWNERSHIP:
+ if (copy_from_user(&u.opal_key, arg, sizeof(u.opal_key))) {
+ ret = -EFAULT;
+ goto out;
+ }
+ ret = opal_take_ownership(dev, &u.opal_key);
+ break;
+ case IOC_OPAL_ACTIVATE_LSP:
+ if (copy_from_user(&u.opal_lr_act, arg, sizeof(u.opal_lr_act))) {
+ ret = -EFAULT;
+ goto out;
+ }
+ ret = opal_activate_lsp(dev, &u.opal_lr_act);
+ break;
+ case IOC_OPAL_SET_PW:
+ if (copy_from_user(&u.opal_pw, arg, sizeof(u.opal_pw))) {
+ ret = -EFAULT;
+ goto out;
+ }
+ ret = opal_set_new_pw(dev, &u.opal_pw);
+ break;
+ case IOC_OPAL_ACTIVATE_USR:
+ if (copy_from_user(&u.session, arg, sizeof(u.session))) {
+ ret = -EFAULT;
+ goto out;
+ }
+ ret = opal_activate_user(dev, &u.session);
+ break;
+ case IOC_OPAL_REVERT_TPR:
+ if (copy_from_user(&u.opal_key, arg, sizeof(u.opal_key))) {
+ ret = -EFAULT;
+ goto out;
+ }
+ ret = opal_reverttper(dev, &u.opal_key);
+ break;
+ case IOC_OPAL_LR_SETUP:
+ if (copy_from_user(&u.lrs, arg, sizeof(u.lrs))) {
+ ret = -EFAULT;
+ goto out;
+ }
+ ret = opal_setup_locking_range(dev, &u.lrs);
+ break;
+ case IOC_OPAL_ADD_USR_TO_LR:
+ if (copy_from_user(&u.lk_unlk, arg, sizeof(u.lk_unlk))) {
+ ret = -EFAULT;
+ goto out;
+ }
+ ret = opal_add_user_to_lr(dev, &u.lk_unlk);
+ break;
+ case IOC_OPAL_ENABLE_DISABLE_MBR:
+ if (copy_from_user(&u.mbr, arg, sizeof(u.mbr))) {
+ ret = -EFAULT;
+ goto out;
+ }
+ ret = opal_enable_disable_shadow_mbr(dev, &u.mbr);
+ break;
+ case IOC_OPAL_ERASE_LR:
+ if (copy_from_user(&u.session, arg, sizeof(u.session))) {
+ ret = -EFAULT;
+ goto out;
+ }
+ ret = opal_erase_locking_range(dev, &u.session);
+ break;
+ case IOC_OPAL_SECURE_ERASE_LR:
+ if (copy_from_user(&u.session, arg, sizeof(u.session))) {
+ ret = -EFAULT;
+ goto out;
+ }
+ ret = opal_secure_erase_locking_range(dev, &u.session);
+ break;
default:
pr_warn("No such Opal Ioctl %u\n", cmd);
}
- return -ENOTTY;
+
+out:
+ mutex_unlock(&ioctl_lock);
+ return ret;
}
EXPORT_SYMBOL_GPL(sed_ioctl);
--
2.7.4
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related
* [PATCH] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN
From: Scott Bauer @ 2017-02-08 22:56 UTC (permalink / raw)
To: linux-nvme
Cc: keith.busch, hch, linux-kernel, axboe, linux-block, Scott Bauer,
jonathan.derrick
When CONFIG_KASAN is enabled, compilation fails:
block/sed-opal.c: In function 'sed_ioctl':
block/sed-opal.c:2447:1: error: the frame size of 2256 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
Moved all the ioctl structures off the stack to a static
union to prevent oversized stack frame size.
Fixes: 455a7b238cd6 ("block: Add Sed-opal library")
Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Scott Bauer <scott.bauer@intel.com>
---
block/sed-opal.c | 187 ++++++++++++++++++++++++++++++-------------------------
1 file changed, 102 insertions(+), 85 deletions(-)
diff --git a/block/sed-opal.c b/block/sed-opal.c
index bf1406e..b88867d 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -2344,8 +2344,21 @@ bool opal_unlock_from_suspend(struct opal_dev *dev)
}
EXPORT_SYMBOL(opal_unlock_from_suspend);
+static union {
+ struct opal_lock_unlock lk_unlk;
+ struct opal_key opal_key;
+ struct opal_lr_act opal_lr_act;
+ struct opal_new_pw opal_pw;
+ struct opal_session_info session;
+ struct opal_user_lr_setup lrs;
+ struct opal_mbr_data mbr;
+} u;
+
+DEFINE_MUTEX(ioctl_lock);
+
int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
{
+ int ret = -ENOTTY;
void __user *arg = (void __user *)ptr;
if (!capable(CAP_SYS_ADMIN))
@@ -2355,94 +2368,98 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
return -ENOTSUPP;
}
+ mutex_lock(&ioctl_lock);
switch (cmd) {
- case IOC_OPAL_SAVE: {
- struct opal_lock_unlock lk_unlk;
-
- if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
- return -EFAULT;
- return opal_save(dev, &lk_unlk);
- }
- case IOC_OPAL_LOCK_UNLOCK: {
- struct opal_lock_unlock lk_unlk;
-
- if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
- return -EFAULT;
- return opal_lock_unlock(dev, &lk_unlk);
- }
- case IOC_OPAL_TAKE_OWNERSHIP: {
- struct opal_key opal_key;
-
- if (copy_from_user(&opal_key, arg, sizeof(opal_key)))
- return -EFAULT;
- return opal_take_ownership(dev, &opal_key);
- }
- case IOC_OPAL_ACTIVATE_LSP: {
- struct opal_lr_act opal_lr_act;
-
- if (copy_from_user(&opal_lr_act, arg, sizeof(opal_lr_act)))
- return -EFAULT;
- return opal_activate_lsp(dev, &opal_lr_act);
- }
- case IOC_OPAL_SET_PW: {
- struct opal_new_pw opal_pw;
-
- if (copy_from_user(&opal_pw, arg, sizeof(opal_pw)))
- return -EFAULT;
- return opal_set_new_pw(dev, &opal_pw);
- }
- case IOC_OPAL_ACTIVATE_USR: {
- struct opal_session_info session;
-
- if (copy_from_user(&session, arg, sizeof(session)))
- return -EFAULT;
- return opal_activate_user(dev, &session);
- }
- case IOC_OPAL_REVERT_TPR: {
- struct opal_key opal_key;
-
- if (copy_from_user(&opal_key, arg, sizeof(opal_key)))
- return -EFAULT;
- return opal_reverttper(dev, &opal_key);
- }
- case IOC_OPAL_LR_SETUP: {
- struct opal_user_lr_setup lrs;
-
- if (copy_from_user(&lrs, arg, sizeof(lrs)))
- return -EFAULT;
- return opal_setup_locking_range(dev, &lrs);
- }
- case IOC_OPAL_ADD_USR_TO_LR: {
- struct opal_lock_unlock lk_unlk;
-
- if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
- return -EFAULT;
- return opal_add_user_to_lr(dev, &lk_unlk);
- }
- case IOC_OPAL_ENABLE_DISABLE_MBR: {
- struct opal_mbr_data mbr;
-
- if (copy_from_user(&mbr, arg, sizeof(mbr)))
- return -EFAULT;
- return opal_enable_disable_shadow_mbr(dev, &mbr);
- }
- case IOC_OPAL_ERASE_LR: {
- struct opal_session_info session;
-
- if (copy_from_user(&session, arg, sizeof(session)))
- return -EFAULT;
- return opal_erase_locking_range(dev, &session);
- }
- case IOC_OPAL_SECURE_ERASE_LR: {
- struct opal_session_info session;
-
- if (copy_from_user(&session, arg, sizeof(session)))
- return -EFAULT;
- return opal_secure_erase_locking_range(dev, &session);
- }
+ case IOC_OPAL_SAVE:
+ if (copy_from_user(&u.lk_unlk, arg, sizeof(u.lk_unlk))) {
+ ret = -EFAULT;
+ goto out;
+ }
+ ret = opal_save(dev, &u.lk_unlk);
+ break;
+ case IOC_OPAL_LOCK_UNLOCK:
+ if (copy_from_user(&u.lk_unlk, arg, sizeof(u.lk_unlk))) {
+ ret = -EFAULT;
+ goto out;
+ }
+ ret = opal_lock_unlock(dev, &u.lk_unlk);
+ break;
+ case IOC_OPAL_TAKE_OWNERSHIP:
+ if (copy_from_user(&u.opal_key, arg, sizeof(u.opal_key))) {
+ ret = -EFAULT;
+ goto out;
+ }
+ ret = opal_take_ownership(dev, &u.opal_key);
+ break;
+ case IOC_OPAL_ACTIVATE_LSP:
+ if (copy_from_user(&u.opal_lr_act, arg, sizeof(u.opal_lr_act))) {
+ ret = -EFAULT;
+ goto out;
+ }
+ ret = opal_activate_lsp(dev, &u.opal_lr_act);
+ break;
+ case IOC_OPAL_SET_PW:
+ if (copy_from_user(&u.opal_pw, arg, sizeof(u.opal_pw))) {
+ ret = -EFAULT;
+ goto out;
+ }
+ ret = opal_set_new_pw(dev, &u.opal_pw);
+ break;
+ case IOC_OPAL_ACTIVATE_USR:
+ if (copy_from_user(&u.session, arg, sizeof(u.session))) {
+ ret = -EFAULT;
+ goto out;
+ }
+ ret = opal_activate_user(dev, &u.session);
+ break;
+ case IOC_OPAL_REVERT_TPR:
+ if (copy_from_user(&u.opal_key, arg, sizeof(u.opal_key))) {
+ ret = -EFAULT;
+ goto out;
+ }
+ ret = opal_reverttper(dev, &u.opal_key);
+ break;
+ case IOC_OPAL_LR_SETUP:
+ if (copy_from_user(&u.lrs, arg, sizeof(u.lrs))) {
+ ret = -EFAULT;
+ goto out;
+ }
+ ret = opal_setup_locking_range(dev, &u.lrs);
+ break;
+ case IOC_OPAL_ADD_USR_TO_LR:
+ if (copy_from_user(&u.lk_unlk, arg, sizeof(u.lk_unlk))) {
+ ret = -EFAULT;
+ goto out;
+ }
+ ret = opal_add_user_to_lr(dev, &u.lk_unlk);
+ break;
+ case IOC_OPAL_ENABLE_DISABLE_MBR:
+ if (copy_from_user(&u.mbr, arg, sizeof(u.mbr))) {
+ ret = -EFAULT;
+ goto out;
+ }
+ ret = opal_enable_disable_shadow_mbr(dev, &u.mbr);
+ break;
+ case IOC_OPAL_ERASE_LR:
+ if (copy_from_user(&u.session, arg, sizeof(u.session))) {
+ ret = -EFAULT;
+ goto out;
+ }
+ ret = opal_erase_locking_range(dev, &u.session);
+ break;
+ case IOC_OPAL_SECURE_ERASE_LR:
+ if (copy_from_user(&u.session, arg, sizeof(u.session))) {
+ ret = -EFAULT;
+ goto out;
+ }
+ ret = opal_secure_erase_locking_range(dev, &u.session);
+ break;
default:
pr_warn("No such Opal Ioctl %u\n", cmd);
}
- return -ENOTTY;
+
+ out:
+ mutex_unlock(&ioctl_lock);
+ return ret;
}
EXPORT_SYMBOL_GPL(sed_ioctl);
--
2.7.4
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related
* Re: [PATCH] block: sed-opal: reduce stack size of ioctl handler
From: Scott Bauer @ 2017-02-08 22:12 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Jens Axboe, Rafael Antognolli, Michael Ellerman, linux-kernel,
linux-nvme, linux-block, Paul Mackerras, Benjamin Herrenschmidt,
linuxppc-dev, hch, Jonathan Derrick
In-Reply-To: <20170208215827.GA9733@sbauer-Z170X-UD5>
On Wed, Feb 08, 2017 at 02:58:28PM -0700, Scott Bauer wrote:
> On Wed, Feb 08, 2017 at 10:15:28PM +0100, Arnd Bergmann wrote:
> > When CONFIG_KASAN is in use, the sed_ioctl function uses unusually large stack,
> > as each possible ioctl argument gets its own stack area plus redzone:
> >
> > block/sed-opal.c: In function 'sed_ioctl':
> > block/sed-opal.c:2447:1: error: the frame size of 2256 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
> >
> > Moving the copy_from_user() calls into the individual functions has little
> > effect on readablility, but significantly reduces the stack size, with the
> > largest individual function (opal_enable_disable_shadow_mbr) now at
> > reasonable 456 bytes.
> >
> > Fixes: 455a7b238cd6 ("block: Add Sed-opal library")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
>
> Hi Arnd,
>
> Thank you for the report. We want to keep the function calls agnostic to userland.
> In the future we will have in-kernel callers and I don't want to have to do any
> get_fs(KERNEL_DS) wizardry.
>
> Instead I think we can use a union to lessen the stack burden. I tested this patch just now
> with config_ksasan and was able to build.
Nack on this patch, it only really masks the issue. Keith pointed out we have a call chain
up to this ioctl then deeper down into nvme then the block layer. If we use 25% of the stack
just for this function it's still too dangerous and we'll run into corruption later on and not
remember this fix. I'll come up with another solution.
^ permalink raw reply
* Re: [PATCH] block: sed-opal: reduce stack size of ioctl handler
From: Scott Bauer @ 2017-02-08 21:58 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Jonathan Derrick, Rafael Antognolli, Jens Axboe,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
linux-nvme, linux-block, linuxppc-dev, linux-kernel, hch
In-Reply-To: <20170208211546.2789607-1-arnd@arndb.de>
On Wed, Feb 08, 2017 at 10:15:28PM +0100, Arnd Bergmann wrote:
> When CONFIG_KASAN is in use, the sed_ioctl function uses unusually large stack,
> as each possible ioctl argument gets its own stack area plus redzone:
>
> block/sed-opal.c: In function 'sed_ioctl':
> block/sed-opal.c:2447:1: error: the frame size of 2256 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
>
> Moving the copy_from_user() calls into the individual functions has little
> effect on readablility, but significantly reduces the stack size, with the
> largest individual function (opal_enable_disable_shadow_mbr) now at
> reasonable 456 bytes.
>
> Fixes: 455a7b238cd6 ("block: Add Sed-opal library")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Hi Arnd,
Thank you for the report. We want to keep the function calls agnostic to userland.
In the future we will have in-kernel callers and I don't want to have to do any
get_fs(KERNEL_DS) wizardry.
Instead I think we can use a union to lessen the stack burden. I tested this patch just now
with config_ksasan and was able to build.
>From dfa6a2c842a6e45cab198c9058e753835a81521e Mon Sep 17 00:00:00 2001
From: Scott Bauer <scott.bauer@intel.com>
Date: Wed, 8 Feb 2017 14:49:32 -0700
Subject: [PATCH] Unionize stack parameters for sed_ioctl to prevent oversized
stack
block/sed-opal.c: In function 'sed_ioctl':
block/sed-opal.c:2447:1: error: the frame size of 2256 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
Moved all the ioctl structures into a union to prevent oversized stack frame size.
Fixes: 455a7b238cd6 ("block: Add Sed-opal library")
Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Scott Bauer <scott.bauer@intel.com>
---
block/sed-opal.c | 118 ++++++++++++++++++++++---------------------------------
1 file changed, 46 insertions(+), 72 deletions(-)
diff --git a/block/sed-opal.c b/block/sed-opal.c
index bf1406e..f509168 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -2347,6 +2347,15 @@ EXPORT_SYMBOL(opal_unlock_from_suspend);
int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
{
void __user *arg = (void __user *)ptr;
+ union {
+ struct opal_lock_unlock lk_unlk;
+ struct opal_key opal_key;
+ struct opal_lr_act opal_lr_act;
+ struct opal_new_pw opal_pw;
+ struct opal_session_info session;
+ struct opal_user_lr_setup lrs;
+ struct opal_mbr_data mbr;
+ }u;
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
@@ -2355,91 +2364,56 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
return -ENOTSUPP;
}
+ memset(&u, 0, sizeof(u));
switch (cmd) {
- case IOC_OPAL_SAVE: {
- struct opal_lock_unlock lk_unlk;
-
- if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
+ case IOC_OPAL_SAVE:
+ if (copy_from_user(&u.lk_unlk, arg, sizeof(u.lk_unlk)))
return -EFAULT;
- return opal_save(dev, &lk_unlk);
- }
- case IOC_OPAL_LOCK_UNLOCK: {
- struct opal_lock_unlock lk_unlk;
-
- if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
+ return opal_save(dev, &u.lk_unlk);
+ case IOC_OPAL_LOCK_UNLOCK:
+ if (copy_from_user(&u.lk_unlk, arg, sizeof(u.lk_unlk)))
return -EFAULT;
- return opal_lock_unlock(dev, &lk_unlk);
- }
- case IOC_OPAL_TAKE_OWNERSHIP: {
- struct opal_key opal_key;
-
- if (copy_from_user(&opal_key, arg, sizeof(opal_key)))
+ return opal_lock_unlock(dev, &u.lk_unlk);
+ case IOC_OPAL_TAKE_OWNERSHIP:
+ if (copy_from_user(&u.opal_key, arg, sizeof(u.opal_key)))
return -EFAULT;
- return opal_take_ownership(dev, &opal_key);
- }
- case IOC_OPAL_ACTIVATE_LSP: {
- struct opal_lr_act opal_lr_act;
-
- if (copy_from_user(&opal_lr_act, arg, sizeof(opal_lr_act)))
+ return opal_take_ownership(dev, &u.opal_key);
+ case IOC_OPAL_ACTIVATE_LSP:
+ if (copy_from_user(&u.opal_lr_act, arg, sizeof(u.opal_lr_act)))
return -EFAULT;
- return opal_activate_lsp(dev, &opal_lr_act);
- }
- case IOC_OPAL_SET_PW: {
- struct opal_new_pw opal_pw;
-
- if (copy_from_user(&opal_pw, arg, sizeof(opal_pw)))
+ return opal_activate_lsp(dev, &u.opal_lr_act);
+ case IOC_OPAL_SET_PW:
+ if (copy_from_user(&u.opal_pw, arg, sizeof(u.opal_pw)))
return -EFAULT;
- return opal_set_new_pw(dev, &opal_pw);
- }
- case IOC_OPAL_ACTIVATE_USR: {
- struct opal_session_info session;
-
- if (copy_from_user(&session, arg, sizeof(session)))
+ return opal_set_new_pw(dev, &u.opal_pw);
+ case IOC_OPAL_ACTIVATE_USR:
+ if (copy_from_user(&u.session, arg, sizeof(u.session)))
return -EFAULT;
- return opal_activate_user(dev, &session);
- }
- case IOC_OPAL_REVERT_TPR: {
- struct opal_key opal_key;
-
- if (copy_from_user(&opal_key, arg, sizeof(opal_key)))
+ return opal_activate_user(dev, &u.session);
+ case IOC_OPAL_REVERT_TPR:
+ if (copy_from_user(&u.opal_key, arg, sizeof(u.opal_key)))
return -EFAULT;
- return opal_reverttper(dev, &opal_key);
- }
- case IOC_OPAL_LR_SETUP: {
- struct opal_user_lr_setup lrs;
-
- if (copy_from_user(&lrs, arg, sizeof(lrs)))
+ return opal_reverttper(dev, &u.opal_key);
+ case IOC_OPAL_LR_SETUP:
+ if (copy_from_user(&u.lrs, arg, sizeof(u.lrs)))
return -EFAULT;
- return opal_setup_locking_range(dev, &lrs);
- }
- case IOC_OPAL_ADD_USR_TO_LR: {
- struct opal_lock_unlock lk_unlk;
-
- if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
+ return opal_setup_locking_range(dev, &u.lrs);
+ case IOC_OPAL_ADD_USR_TO_LR:
+ if (copy_from_user(&u.lk_unlk, arg, sizeof(u.lk_unlk)))
return -EFAULT;
- return opal_add_user_to_lr(dev, &lk_unlk);
- }
- case IOC_OPAL_ENABLE_DISABLE_MBR: {
- struct opal_mbr_data mbr;
-
- if (copy_from_user(&mbr, arg, sizeof(mbr)))
+ return opal_add_user_to_lr(dev, &u.lk_unlk);
+ case IOC_OPAL_ENABLE_DISABLE_MBR:
+ if (copy_from_user(&u.mbr, arg, sizeof(u.mbr)))
return -EFAULT;
- return opal_enable_disable_shadow_mbr(dev, &mbr);
- }
- case IOC_OPAL_ERASE_LR: {
- struct opal_session_info session;
-
- if (copy_from_user(&session, arg, sizeof(session)))
+ return opal_enable_disable_shadow_mbr(dev, &u.mbr);
+ case IOC_OPAL_ERASE_LR:
+ if (copy_from_user(&u.session, arg, sizeof(u.session)))
return -EFAULT;
- return opal_erase_locking_range(dev, &session);
- }
- case IOC_OPAL_SECURE_ERASE_LR: {
- struct opal_session_info session;
-
- if (copy_from_user(&session, arg, sizeof(session)))
+ return opal_erase_locking_range(dev, &u.session);
+ case IOC_OPAL_SECURE_ERASE_LR:
+ if (copy_from_user(&u.session, arg, sizeof(u.session)))
return -EFAULT;
- return opal_secure_erase_locking_range(dev, &session);
- }
+ return opal_secure_erase_locking_range(dev, &u.session);
default:
pr_warn("No such Opal Ioctl %u\n", cmd);
}
--
2.7.4
^ permalink raw reply related
* [PATCH] block: sed-opal: reduce stack size of ioctl handler
From: Arnd Bergmann @ 2017-02-08 21:15 UTC (permalink / raw)
To: Scott Bauer, Jonathan Derrick, Rafael Antognolli
Cc: Arnd Bergmann, Jens Axboe, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, linux-nvme, linux-block, linuxppc-dev,
linux-kernel
When CONFIG_KASAN is in use, the sed_ioctl function uses unusually large stack,
as each possible ioctl argument gets its own stack area plus redzone:
block/sed-opal.c: In function 'sed_ioctl':
block/sed-opal.c:2447:1: error: the frame size of 2256 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
Moving the copy_from_user() calls into the individual functions has little
effect on readablility, but significantly reduces the stack size, with the
largest individual function (opal_enable_disable_shadow_mbr) now at
reasonable 456 bytes.
Fixes: 455a7b238cd6 ("block: Add Sed-opal library")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
block/sed-opal.c | 256 +++++++++++++++++++++++++++----------------------------
1 file changed, 128 insertions(+), 128 deletions(-)
diff --git a/block/sed-opal.c b/block/sed-opal.c
index bf1406e5159b..6b33d970ce5e 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -1957,7 +1957,7 @@ void init_opal_dev(struct opal_dev *opal_dev, sec_send_recv *send_recv)
EXPORT_SYMBOL(init_opal_dev);
static int opal_secure_erase_locking_range(struct opal_dev *dev,
- struct opal_session_info *opal_session)
+ struct opal_session_info __user *arg)
{
void *data[3] = { NULL };
static const opal_step erase_funcs[] = {
@@ -1968,14 +1968,18 @@ static int opal_secure_erase_locking_range(struct opal_dev *dev,
end_opal_session,
NULL,
};
+ struct opal_session_info opal_session;
int ret;
+ if (copy_from_user(&opal_session, arg, sizeof(opal_session)))
+ return -EFAULT;
+
mutex_lock(&dev->dev_lock);
setup_opal_dev(dev, erase_funcs);
dev->func_data = data;
- dev->func_data[1] = opal_session;
- dev->func_data[2] = &opal_session->opal_key.lr;
+ dev->func_data[1] = &opal_session;
+ dev->func_data[2] = &opal_session.opal_key.lr;
ret = next(dev);
mutex_unlock(&dev->dev_lock);
@@ -1983,7 +1987,7 @@ static int opal_secure_erase_locking_range(struct opal_dev *dev,
}
static int opal_erase_locking_range(struct opal_dev *dev,
- struct opal_session_info *opal_session)
+ struct opal_session_info __user *arg)
{
void *data[3] = { NULL };
static const opal_step erase_funcs[] = {
@@ -1993,14 +1997,18 @@ static int opal_erase_locking_range(struct opal_dev *dev,
end_opal_session,
NULL,
};
+ struct opal_session_info opal_session;
int ret;
+ if (copy_from_user(&opal_session, arg, sizeof(opal_session)))
+ return -EFAULT;
+
mutex_lock(&dev->dev_lock);
setup_opal_dev(dev, erase_funcs);
dev->func_data = data;
- dev->func_data[1] = opal_session;
- dev->func_data[2] = opal_session;
+ dev->func_data[1] = &opal_session;
+ dev->func_data[2] = &opal_session;
ret = next(dev);
mutex_unlock(&dev->dev_lock);
@@ -2008,7 +2016,7 @@ static int opal_erase_locking_range(struct opal_dev *dev,
}
static int opal_enable_disable_shadow_mbr(struct opal_dev *dev,
- struct opal_mbr_data *opal_mbr)
+ struct opal_mbr_data __user *arg)
{
void *func_data[6] = { NULL };
static const opal_step mbr_funcs[] = {
@@ -2021,34 +2029,42 @@ static int opal_enable_disable_shadow_mbr(struct opal_dev *dev,
end_opal_session,
NULL,
};
+ struct opal_mbr_data opal_mbr;
int ret;
- if (opal_mbr->enable_disable != OPAL_MBR_ENABLE &&
- opal_mbr->enable_disable != OPAL_MBR_DISABLE)
+ if (copy_from_user(&opal_mbr, arg, sizeof(opal_mbr)))
+ return -EFAULT;
+
+ if (opal_mbr.enable_disable != OPAL_MBR_ENABLE &&
+ opal_mbr.enable_disable != OPAL_MBR_DISABLE)
return -EINVAL;
mutex_lock(&dev->dev_lock);
setup_opal_dev(dev, mbr_funcs);
dev->func_data = func_data;
- dev->func_data[1] = &opal_mbr->key;
- dev->func_data[2] = &opal_mbr->enable_disable;
- dev->func_data[4] = &opal_mbr->key;
- dev->func_data[5] = &opal_mbr->enable_disable;
+ dev->func_data[1] = &opal_mbr.key;
+ dev->func_data[2] = &opal_mbr.enable_disable;
+ dev->func_data[4] = &opal_mbr.key;
+ dev->func_data[5] = &opal_mbr.enable_disable;
ret = next(dev);
mutex_unlock(&dev->dev_lock);
return ret;
}
-static int opal_save(struct opal_dev *dev, struct opal_lock_unlock *lk_unlk)
+static int opal_save(struct opal_dev *dev, struct opal_lock_unlock __user *arg)
{
struct opal_suspend_data *suspend;
+ struct opal_lock_unlock lk_unlk;
+
+ if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
+ return -EFAULT;
suspend = kzalloc(sizeof(*suspend), GFP_KERNEL);
if (!suspend)
return -ENOMEM;
- suspend->unlk = *lk_unlk;
- suspend->lr = lk_unlk->session.opal_key.lr;
+ suspend->unlk = lk_unlk;
+ suspend->lr = lk_unlk.session.opal_key.lr;
mutex_lock(&dev->dev_lock);
setup_opal_dev(dev, NULL);
@@ -2058,7 +2074,7 @@ static int opal_save(struct opal_dev *dev, struct opal_lock_unlock *lk_unlk)
}
static int opal_add_user_to_lr(struct opal_dev *dev,
- struct opal_lock_unlock *lk_unlk)
+ struct opal_lock_unlock *arg)
{
void *func_data[3] = { NULL };
static const opal_step funcs[] = {
@@ -2068,20 +2084,24 @@ static int opal_add_user_to_lr(struct opal_dev *dev,
end_opal_session,
NULL
};
+ struct opal_lock_unlock lk_unlk;
int ret;
- if (lk_unlk->l_state != OPAL_RO &&
- lk_unlk->l_state != OPAL_RW) {
+ if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
+ return -EFAULT;
+
+ if (lk_unlk.l_state != OPAL_RO &&
+ lk_unlk.l_state != OPAL_RW) {
pr_err("Locking state was not RO or RW\n");
return -EINVAL;
}
- if (lk_unlk->session.who < OPAL_USER1 &&
- lk_unlk->session.who > OPAL_USER9) {
+ if (lk_unlk.session.who < OPAL_USER1 &&
+ lk_unlk.session.who > OPAL_USER9) {
pr_err("Authority was not within the range of users: %d\n",
- lk_unlk->session.who);
+ lk_unlk.session.who);
return -EINVAL;
}
- if (lk_unlk->session.sum) {
+ if (lk_unlk.session.sum) {
pr_err("%s not supported in sum. Use setup locking range\n",
__func__);
return -EINVAL;
@@ -2090,14 +2110,14 @@ static int opal_add_user_to_lr(struct opal_dev *dev,
mutex_lock(&dev->dev_lock);
setup_opal_dev(dev, funcs);
dev->func_data = func_data;
- dev->func_data[1] = &lk_unlk->session.opal_key;
- dev->func_data[2] = lk_unlk;
+ dev->func_data[1] = &lk_unlk.session.opal_key;
+ dev->func_data[2] = &lk_unlk;
ret = next(dev);
mutex_unlock(&dev->dev_lock);
return ret;
}
-static int opal_reverttper(struct opal_dev *dev, struct opal_key *opal)
+static int opal_reverttper(struct opal_dev *dev, struct opal_key __user *arg)
{
void *data[2] = { NULL };
static const opal_step revert_funcs[] = {
@@ -2106,12 +2126,16 @@ static int opal_reverttper(struct opal_dev *dev, struct opal_key *opal)
revert_tper, /* controller will terminate session */
NULL,
};
+ struct opal_key opal_key;
int ret;
+ if (copy_from_user(&opal_key, arg, sizeof(opal_key)))
+ return -EFAULT;
+
mutex_lock(&dev->dev_lock);
setup_opal_dev(dev, revert_funcs);
dev->func_data = data;
- dev->func_data[1] = opal;
+ dev->func_data[1] = &opal_key;
ret = next(dev);
mutex_unlock(&dev->dev_lock);
return ret;
@@ -2145,22 +2169,26 @@ static int __opal_lock_unlock(struct opal_dev *dev)
return next(dev);
}
-static int opal_lock_unlock(struct opal_dev *dev, struct opal_lock_unlock *lk_unlk)
+static int opal_lock_unlock(struct opal_dev *dev, struct opal_lock_unlock __user *arg)
{
+ struct opal_lock_unlock lk_unlk;
void *func_data[3] = { NULL };
int ret;
- if (lk_unlk->session.who < OPAL_ADMIN1 ||
- lk_unlk->session.who > OPAL_USER9)
+ if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
+ return -EFAULT;
+
+ if (lk_unlk.session.who < OPAL_ADMIN1 ||
+ lk_unlk.session.who > OPAL_USER9)
return -EINVAL;
mutex_lock(&dev->dev_lock);
setup_opal_dev(dev, NULL);
dev->func_data = func_data;
- dev->func_data[1] = &lk_unlk->session;
- dev->func_data[2] = lk_unlk;
+ dev->func_data[1] = &lk_unlk.session;
+ dev->func_data[2] = &lk_unlk;
- if (lk_unlk->session.sum)
+ if (lk_unlk.session.sum)
ret = __opal_lock_unlock_sum(dev);
else
ret = __opal_lock_unlock(dev);
@@ -2169,7 +2197,7 @@ static int opal_lock_unlock(struct opal_dev *dev, struct opal_lock_unlock *lk_un
return ret;
}
-static int opal_take_ownership(struct opal_dev *dev, struct opal_key *opal)
+static int opal_take_ownership(struct opal_dev *dev, struct opal_key __user *arg)
{
static const opal_step owner_funcs[] = {
opal_discovery0,
@@ -2182,22 +2210,26 @@ static int opal_take_ownership(struct opal_dev *dev, struct opal_key *opal)
NULL
};
void *data[6] = { NULL };
+ struct opal_key opal;
int ret;
+ if (copy_from_user(&opal, arg, sizeof(opal)))
+ return -EFAULT;
+
if (!dev)
return -ENODEV;
mutex_lock(&dev->dev_lock);
setup_opal_dev(dev, owner_funcs);
dev->func_data = data;
- dev->func_data[4] = opal;
- dev->func_data[5] = opal;
+ dev->func_data[4] = &opal;
+ dev->func_data[5] = &opal;
ret = next(dev);
mutex_unlock(&dev->dev_lock);
return ret;
}
-static int opal_activate_lsp(struct opal_dev *dev, struct opal_lr_act *opal_lr_act)
+static int opal_activate_lsp(struct opal_dev *dev, struct opal_lr_act __user *arg)
{
void *data[4] = { NULL };
static const opal_step active_funcs[] = {
@@ -2208,23 +2240,27 @@ static int opal_activate_lsp(struct opal_dev *dev, struct opal_lr_act *opal_lr_a
end_opal_session,
NULL
};
+ struct opal_lr_act opal_lr_act;
int ret;
- if (!opal_lr_act->num_lrs || opal_lr_act->num_lrs > OPAL_MAX_LRS)
+ if (copy_from_user(&opal_lr_act, arg, sizeof(opal_lr_act)))
+ return -EFAULT;
+
+ if (!opal_lr_act.num_lrs || opal_lr_act.num_lrs > OPAL_MAX_LRS)
return -EINVAL;
mutex_lock(&dev->dev_lock);
setup_opal_dev(dev, active_funcs);
dev->func_data = data;
- dev->func_data[1] = &opal_lr_act->key;
- dev->func_data[3] = opal_lr_act;
+ dev->func_data[1] = &opal_lr_act.key;
+ dev->func_data[3] = &opal_lr_act;
ret = next(dev);
mutex_unlock(&dev->dev_lock);
return ret;
}
static int opal_setup_locking_range(struct opal_dev *dev,
- struct opal_user_lr_setup *opal_lrs)
+ struct opal_user_lr_setup __user *arg)
{
void *data[3] = { NULL };
static const opal_step lr_funcs[] = {
@@ -2234,19 +2270,23 @@ static int opal_setup_locking_range(struct opal_dev *dev,
end_opal_session,
NULL,
};
+ struct opal_user_lr_setup opal_lrs;
int ret;
+ if (copy_from_user(&opal_lrs, arg, sizeof(opal_lrs)))
+ return -EFAULT;
+
mutex_lock(&dev->dev_lock);
setup_opal_dev(dev, lr_funcs);
dev->func_data = data;
- dev->func_data[1] = &opal_lrs->session;
- dev->func_data[2] = opal_lrs;
+ dev->func_data[1] = &opal_lrs.session;
+ dev->func_data[2] = &opal_lrs;
ret = next(dev);
mutex_unlock(&dev->dev_lock);
return ret;
}
-static int opal_set_new_pw(struct opal_dev *dev, struct opal_new_pw *opal_pw)
+static int opal_set_new_pw(struct opal_dev *dev, struct opal_new_pw __user *arg)
{
static const opal_step pw_funcs[] = {
opal_discovery0,
@@ -2256,19 +2296,23 @@ static int opal_set_new_pw(struct opal_dev *dev, struct opal_new_pw *opal_pw)
NULL
};
void *data[3] = { NULL };
+ struct opal_new_pw opal_pw;
int ret;
- if (opal_pw->session.who < OPAL_ADMIN1 ||
- opal_pw->session.who > OPAL_USER9 ||
- opal_pw->new_user_pw.who < OPAL_ADMIN1 ||
- opal_pw->new_user_pw.who > OPAL_USER9)
+ if (copy_from_user(&opal_pw, arg, sizeof(opal_pw)))
+ return -EFAULT;
+
+ if (opal_pw.session.who < OPAL_ADMIN1 ||
+ opal_pw.session.who > OPAL_USER9 ||
+ opal_pw.new_user_pw.who < OPAL_ADMIN1 ||
+ opal_pw.new_user_pw.who > OPAL_USER9)
return -EINVAL;
mutex_lock(&dev->dev_lock);
setup_opal_dev(dev, pw_funcs);
dev->func_data = data;
- dev->func_data[1] = (void *) &opal_pw->session;
- dev->func_data[2] = (void *) &opal_pw->new_user_pw;
+ dev->func_data[1] = (void *) &opal_pw.session;
+ dev->func_data[2] = (void *) &opal_pw.new_user_pw;
ret = next(dev);
mutex_unlock(&dev->dev_lock);
@@ -2276,7 +2320,7 @@ static int opal_set_new_pw(struct opal_dev *dev, struct opal_new_pw *opal_pw)
}
static int opal_activate_user(struct opal_dev *dev,
- struct opal_session_info *opal_session)
+ struct opal_session_info __user *arg)
{
static const opal_step act_funcs[] = {
opal_discovery0,
@@ -2286,20 +2330,24 @@ static int opal_activate_user(struct opal_dev *dev,
NULL
};
void *data[3] = { NULL };
+ struct opal_session_info opal_session;
int ret;
+ if (copy_from_user(&opal_session, arg, sizeof(opal_session)))
+ return -EFAULT;
+
/* We can't activate Admin1 it's active as manufactured */
- if (opal_session->who < OPAL_USER1 &&
- opal_session->who > OPAL_USER9) {
- pr_err("Who was not a valid user: %d\n", opal_session->who);
+ if (opal_session.who < OPAL_USER1 &&
+ opal_session.who > OPAL_USER9) {
+ pr_err("Who was not a valid user: %d\n", opal_session.who);
return -EINVAL;
}
mutex_lock(&dev->dev_lock);
setup_opal_dev(dev, act_funcs);
dev->func_data = data;
- dev->func_data[1] = &opal_session->opal_key;
- dev->func_data[2] = opal_session;
+ dev->func_data[1] = &opal_session.opal_key;
+ dev->func_data[2] = &opal_session;
ret = next(dev);
mutex_unlock(&dev->dev_lock);
return ret;
@@ -2356,90 +2404,42 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
}
switch (cmd) {
- case IOC_OPAL_SAVE: {
- struct opal_lock_unlock lk_unlk;
+ case IOC_OPAL_SAVE:
+ return opal_save(dev, arg);
- if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
- return -EFAULT;
- return opal_save(dev, &lk_unlk);
- }
- case IOC_OPAL_LOCK_UNLOCK: {
- struct opal_lock_unlock lk_unlk;
+ case IOC_OPAL_LOCK_UNLOCK:
+ return opal_lock_unlock(dev, arg);
- if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
- return -EFAULT;
- return opal_lock_unlock(dev, &lk_unlk);
- }
- case IOC_OPAL_TAKE_OWNERSHIP: {
- struct opal_key opal_key;
+ case IOC_OPAL_TAKE_OWNERSHIP:
+ return opal_take_ownership(dev, arg);
- if (copy_from_user(&opal_key, arg, sizeof(opal_key)))
- return -EFAULT;
- return opal_take_ownership(dev, &opal_key);
- }
- case IOC_OPAL_ACTIVATE_LSP: {
- struct opal_lr_act opal_lr_act;
+ case IOC_OPAL_ACTIVATE_LSP:
+ return opal_activate_lsp(dev, arg);
- if (copy_from_user(&opal_lr_act, arg, sizeof(opal_lr_act)))
- return -EFAULT;
- return opal_activate_lsp(dev, &opal_lr_act);
- }
- case IOC_OPAL_SET_PW: {
- struct opal_new_pw opal_pw;
+ case IOC_OPAL_SET_PW:
+ return opal_set_new_pw(dev, arg);
- if (copy_from_user(&opal_pw, arg, sizeof(opal_pw)))
- return -EFAULT;
- return opal_set_new_pw(dev, &opal_pw);
- }
- case IOC_OPAL_ACTIVATE_USR: {
- struct opal_session_info session;
+ case IOC_OPAL_ACTIVATE_USR:
+ return opal_activate_user(dev, arg);
- if (copy_from_user(&session, arg, sizeof(session)))
- return -EFAULT;
- return opal_activate_user(dev, &session);
- }
- case IOC_OPAL_REVERT_TPR: {
- struct opal_key opal_key;
+ case IOC_OPAL_REVERT_TPR:
+ return opal_reverttper(dev, arg);
- if (copy_from_user(&opal_key, arg, sizeof(opal_key)))
- return -EFAULT;
- return opal_reverttper(dev, &opal_key);
- }
- case IOC_OPAL_LR_SETUP: {
- struct opal_user_lr_setup lrs;
+ case IOC_OPAL_LR_SETUP:
+ return opal_setup_locking_range(dev, arg);
- if (copy_from_user(&lrs, arg, sizeof(lrs)))
- return -EFAULT;
- return opal_setup_locking_range(dev, &lrs);
- }
- case IOC_OPAL_ADD_USR_TO_LR: {
- struct opal_lock_unlock lk_unlk;
+ case IOC_OPAL_ADD_USR_TO_LR:
+ return opal_add_user_to_lr(dev, arg);
- if (copy_from_user(&lk_unlk, arg, sizeof(lk_unlk)))
- return -EFAULT;
- return opal_add_user_to_lr(dev, &lk_unlk);
- }
- case IOC_OPAL_ENABLE_DISABLE_MBR: {
- struct opal_mbr_data mbr;
+ case IOC_OPAL_ENABLE_DISABLE_MBR:
+ return opal_enable_disable_shadow_mbr(dev, arg);
- if (copy_from_user(&mbr, arg, sizeof(mbr)))
- return -EFAULT;
- return opal_enable_disable_shadow_mbr(dev, &mbr);
- }
- case IOC_OPAL_ERASE_LR: {
- struct opal_session_info session;
+ case IOC_OPAL_ERASE_LR:
+ return opal_erase_locking_range(dev, arg);
- if (copy_from_user(&session, arg, sizeof(session)))
- return -EFAULT;
- return opal_erase_locking_range(dev, &session);
- }
- case IOC_OPAL_SECURE_ERASE_LR: {
- struct opal_session_info session;
+ case IOC_OPAL_SECURE_ERASE_LR:
+ return opal_secure_erase_locking_range(dev, arg);
- if (copy_from_user(&session, arg, sizeof(session)))
- return -EFAULT;
- return opal_secure_erase_locking_range(dev, &session);
- }
default:
pr_warn("No such Opal Ioctl %u\n", cmd);
}
--
2.9.0
^ permalink raw reply related
* Re: Boot regression (was "Re: [PATCH] genhd: Do not hold event lock when scheduling workqueue elements")
From: hch @ 2017-02-08 18:03 UTC (permalink / raw)
To: Jens Axboe
Cc: Dexuan Cui, Bart Van Assche, hare@suse.com, hare@suse.de,
Martin K. Petersen, hch@lst.de, linux-kernel@vger.kernel.org,
linux-block@vger.kernel.org, jth@kernel.org
In-Reply-To: <b21e5580-ed75-6150-3c83-43ecbb0292a5@kernel.dk>
On Wed, Feb 08, 2017 at 10:43:59AM -0700, Jens Axboe wrote:
> I've changed the subject line, this issue has nothing to do with the
> issue that Hannes was attempting to fix.
Nothing really useful in the thread. Dexuan, can you throw in some
prints to see which command times out?
^ permalink raw reply
* Re: support for multi-range discard requests V3
From: Jens Axboe @ 2017-02-08 17:45 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-block, linux-nvme
In-Reply-To: <20170208134650.5380-1-hch@lst.de>
On 02/08/2017 06:46 AM, Christoph Hellwig wrote:
> Hi all,
>
> this series adds support for merging discontiguous discard bios into a
> single request if the driver supports it. This reduces the number of
> discards sent to the device by about a factor of 5-6 for typical
> workloads on NVMe, and for slower devices that use I/O scheduling
> the number will probably be even bigger but I've not implemented
> that support yet.
This looks good to me. It's running testing now, but provided that
the tests are successful, we should get this queued up for 4.11.
--
Jens Axboe
^ permalink raw reply
* Boot regression (was "Re: [PATCH] genhd: Do not hold event lock when scheduling workqueue elements")
From: Jens Axboe @ 2017-02-08 17:43 UTC (permalink / raw)
To: Dexuan Cui, Bart Van Assche, hare@suse.com, hare@suse.de,
Martin K. Petersen
Cc: hch@lst.de, linux-kernel@vger.kernel.org,
linux-block@vger.kernel.org, jth@kernel.org
In-Reply-To: <MWHPR03MB26697302F28F9C2C7D7390D4BF420@MWHPR03MB2669.namprd03.prod.outlook.com>
On 02/08/2017 03:48 AM, Dexuan Cui wrote:
>> From: Jens Axboe [mailto:axboe@kernel.dk]
>> Sent: Wednesday, February 8, 2017 00:09
>> To: Dexuan Cui <decui@microsoft.com>; Bart Van Assche
>> <Bart.VanAssche@sandisk.com>; hare@suse.com; hare@suse.de
>> Cc: hch@lst.de; linux-kernel@vger.kernel.org; linux-block@vger.kernel.org;
>> jth@kernel.org
>> Subject: Re: [PATCH] genhd: Do not hold event lock when scheduling workqueue
>> elements
>>
>> On 02/06/2017 11:29 PM, Dexuan Cui wrote:
>>>> From: linux-block-owner@vger.kernel.org [mailto:linux-block-
>>>> owner@vger.kernel.org] On Behalf Of Dexuan Cui
>>>> with the linux-next kernel.
>>>>
>>>> I can boot the guest with linux-next's next-20170130 without any issue,
>>>> but since next-20170131 I haven't succeeded in booting the guest.
>>>>
>>>> With next-20170203 (mentioned in my mail last Friday), I got the same
>>>> calltrace as Hannes.
>>>>
>>>> With today's linux-next (next-20170206), actually the calltrace changed to
>>>> the below.
>>>> [ 122.023036] ? remove_wait_queue+0x70/0x70
>>>> [ 122.051383] async_synchronize_full+0x17/0x20
>>>> [ 122.076925] do_init_module+0xc1/0x1f9
>>>> [ 122.097530] load_module+0x24bc/0x2980
>>>
>>> I don't know why it hangs here, but this is the same calltrace in my
>>> last-Friday mail, which contains 2 calltraces. It looks the other calltrace has
>>> been resolved by some changes between next-20170203 and today.
>>>
>>> Here the kernel is trying to load the Hyper-V storage driver (hv_storvsc), and
>>> the driver's __init and .probe have finished successfully and then the kernel
>>> hangs here.
>>>
>>> I believe something is broken recently, because I don't have any issue before
>>> Jan 31.
>>
>> Can you try and bisect it?
>>
>> Jens Axboe
>
> I bisected it on the branch for-4.11/next of the linux-block repo and the log shows
> the first bad commit is
> [e9c787e6] scsi: allocate scsi_cmnd structures as part of struct request
>
> # git bisect log
> git bisect start
> # bad: [80c6b15732f0d8830032149cbcbc8d67e074b5e8] blk-mq-sched: (un)register elevator when (un)registering queue
> git bisect bad 80c6b15732f0d8830032149cbcbc8d67e074b5e8
> # good: [309bd96af9e26da3038661bf5cdad780eef49dd9] md: cleanup bio op / flags handling in raid1_write_request
> git bisect good 309bd96af9e26da3038661bf5cdad780eef49dd9
> # bad: [27410a8927fb89bd150de08d749a8ed7f67b7739] nbd: remove REQ_TYPE_DRV_PRIV leftovers
> git bisect bad 27410a8927fb89bd150de08d749a8ed7f67b7739
> # bad: [e9c787e65c0c36529745be47d490d998b4b6e589] scsi: allocate scsi_cmnd structures as part of struct request
> git bisect bad e9c787e65c0c36529745be47d490d998b4b6e589
> # good: [3278255741326b6d66d8ca7d1cb2c57633ee43d9] scsi_dh_rdac: switch to scsi_execute_req_flags()
> git bisect good 3278255741326b6d66d8ca7d1cb2c57633ee43d9
> # good: [0fbc3e0ff623f1012e7c2af96e781eeb26bcc0d7] scsi: remove gfp_flags member in scsi_host_cmd_pool
> git bisect good 0fbc3e0ff623f1012e7c2af96e781eeb26bcc0d7
> # good: [eeff68c5618c8d0920b14533c70b2df007bd94b4] scsi: remove scsi_cmd_dma_pool
> git bisect good eeff68c5618c8d0920b14533c70b2df007bd94b4
> # good: [d48777a633d6fa7ccde0f0e6509f0c01fbfc5299] scsi: remove __scsi_alloc_queue
> git bisect good d48777a633d6fa7ccde0f0e6509f0c01fbfc5299
> # first bad commit: [e9c787e65c0c36529745be47d490d998b4b6e589] scsi: allocate scsi_cmnd structures as part of struct request
Christoph?
I've changed the subject line, this issue has nothing to do with the
issue that Hannes was attempting to fix.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH] bfq-mq: cause deadlock by executing exit_icq body immediately
From: Omar Sandoval @ 2017-02-08 17:17 UTC (permalink / raw)
To: Paolo Valente
Cc: Jens Axboe, Tejun Heo, linux-block, Linux-Kernal, Ulf Hansson,
Linus Walleij, broonie
In-Reply-To: <49CC2920-EF8D-4B9B-B6BE-1722156AAB70@linaro.org>
On Wed, Feb 08, 2017 at 11:39:24AM +0100, Paolo Valente wrote:
>
> > Il giorno 08 feb 2017, alle ore 11:33, Omar Sandoval <osandov@osandov.com> ha scritto:
> >
> > On Wed, Feb 08, 2017 at 11:03:01AM +0100, Paolo Valente wrote:
> >>
> >>> Il giorno 07 feb 2017, alle ore 22:45, Omar Sandoval <osandov@osandov.com> ha scritto:
> >>>
> >>> On Tue, Feb 07, 2017 at 06:33:46PM +0100, Paolo Valente wrote:
> >>>> Hi,
> >>>> this patch is meant to show that, if the body of the hook exit_icq is executed
> >>>> from inside that hook, and not as deferred work, then a circular deadlock
> >>>> occurs.
> >>>>
> >>>> It happens if, on a CPU
> >>>> - the body of icq_exit takes the scheduler lock,
> >>>> - it does so from inside the exit_icq hook, which is invoked with the queue
> >>>> lock held
> >>>>
> >>>> while, on another CPU
> >>>> - bfq_bio_merge, after taking the scheduler lock, invokes bfq_bic_lookup,
> >>>> which, in its turn, takes the queue lock. bfq_bic_lookup needs to take such a
> >>>> lock, because it invokes ioc_lookup_icq.
> >>>>
> >>>> For more details, here is a lockdep report, right before the deadlock did occur.
> >>>>
> >>>> [ 44.059877] ======================================================
> >>>> [ 44.124922] [ INFO: possible circular locking dependency detected ]
> >>>> [ 44.125795] 4.10.0-rc5-bfq-mq+ #38 Not tainted
> >>>> [ 44.126414] -------------------------------------------------------
> >>>> [ 44.127291] sync/2043 is trying to acquire lock:
> >>>> [ 44.128918] (&(&bfqd->lock)->rlock){-.-...}, at: [<ffffffff90484195>] bfq_exit_icq_bfqq+0x55/0x140
> >>>> [ 44.134052]
> >>>> [ 44.134052] but task is already holding lock:
> >>>> [ 44.134868] (&(&q->__queue_lock)->rlock){-.....}, at: [<ffffffff9044738e>] put_io_context_active+0x6e/0xc0
> >>>
> >>> Hey, Paolo,
> >>>
> >>> I only briefly skimmed the code, but what are you using the queue_lock
> >>> for? You should just use your scheduler lock everywhere. blk-mq doesn't
> >>> use the queue lock, so the scheduler is the only thing you need mutual
> >>> exclusion against.
> >>
> >> Hi Omar,
> >> the cause of the problem is that the hook functions bfq_request_merge
> >> and bfq_allow_bio_merge invoke, directly or through other functions,
> >> the function bfq_bic_lookup, which, in its turn, invokes
> >> ioc_lookup_icq. The latter must be invoked with the queue lock held.
> >> In particular the offending lines in bfq_bic_lookup are:
> >>
> >> spin_lock_irqsave(q->queue_lock, flags);
> >> icq = icq_to_bic(ioc_lookup_icq(ioc, q));
> >> spin_unlock_irqrestore(q->queue_lock, flags);
> >>
> >> Maybe I'm missing something and we can avoid taking this lock?
> >
> > Ah, I didn't realize we still used the q->queue_lock for the icq stuff.
> > You're right, you still need that lock for ioc_lookup_icq(). Unless
> > there's something else I'm forgetting, that should be the only thing you
> > need it for in the core code, and you should use your scheduler lock for
> > everything else. What else are you using q->queue_lock for?
>
> Nothing. The deadlock follows from that bfq_request_merge gets called
> with the scheduler lock already held. Problematic paths start from:
> bfq_bio_merge and bfq_insert_request.
>
> I'm trying to understand whether I/we can reorder operations in some
> way that avoids the nested locking, but at no avail so far.
>
> Thanks,
> Paolo
Okay, I understand what you're saying now. It was all in the first email
but I didn't see it right away, sorry about that.
I don't think it makes sense for ->exit_icq() to be invoked while
holding q->queue_lock for blk-mq -- we don't hold that lock for any of
the other hooks. Could you try the below? I haven't convinced myself
that there isn't a circular locking dependency between bfqd->lock and
ioc->lock now, but it's probably easiest for you to just try it.
diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index fe186a9eade9..b12f9c87b4c3 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -35,7 +35,10 @@ static void icq_free_icq_rcu(struct rcu_head *head)
kmem_cache_free(icq->__rcu_icq_cache, icq);
}
-/* Exit an icq. Called with both ioc and q locked. */
+/*
+ * Exit an icq. Called with both ioc and q locked for sq, only ioc locked for
+ * mq.
+ */
static void ioc_exit_icq(struct io_cq *icq)
{
struct elevator_type *et = icq->q->elevator->type;
@@ -166,6 +169,7 @@ EXPORT_SYMBOL(put_io_context);
*/
void put_io_context_active(struct io_context *ioc)
{
+ struct elevator_type *et;
unsigned long flags;
struct io_cq *icq;
@@ -184,13 +188,19 @@ void put_io_context_active(struct io_context *ioc)
hlist_for_each_entry(icq, &ioc->icq_list, ioc_node) {
if (icq->flags & ICQ_EXITED)
continue;
- if (spin_trylock(icq->q->queue_lock)) {
+
+ et = icq->q->elevator->type;
+ if (et->uses_mq) {
ioc_exit_icq(icq);
- spin_unlock(icq->q->queue_lock);
} else {
- spin_unlock_irqrestore(&ioc->lock, flags);
- cpu_relax();
- goto retry;
+ if (spin_trylock(icq->q->queue_lock)) {
+ ioc_exit_icq(icq);
+ spin_unlock(icq->q->queue_lock);
+ } else {
+ spin_unlock_irqrestore(&ioc->lock, flags);
+ cpu_relax();
+ goto retry;
+ }
}
}
spin_unlock_irqrestore(&ioc->lock, flags);
^ permalink raw reply related
* Re: v4.9, 4.4-final: 28 bioset threads on small notebook, 36 threads on cellphone
From: Mike Snitzer @ 2017-02-08 16:34 UTC (permalink / raw)
To: Kent Overstreet
Cc: Pavel Machek, kernel list, axboe, hch, neilb, martin.petersen,
dpark, ming.l, dm-devel, ming.lei, agk, jkosina, geoff, jim,
pjk1939, minchan, ngupta, oleg.drokin, andreas.dilger,
linux-block
In-Reply-To: <20170207204510.qr2l2rg42ez2hobh@moria.home.lan>
On Tue, Feb 07 2017 at 11:58pm -0500,
Kent Overstreet <kent.overstreet@gmail.com> wrote:
> On Tue, Feb 07, 2017 at 09:39:11PM +0100, Pavel Machek wrote:
> > On Mon 2017-02-06 17:49:06, Kent Overstreet wrote:
> > > On Mon, Feb 06, 2017 at 04:47:24PM -0900, Kent Overstreet wrote:
> > > > On Mon, Feb 06, 2017 at 01:53:09PM +0100, Pavel Machek wrote:
> > > > > Still there on v4.9, 36 threads on nokia n900 cellphone.
> > > > >
> > > > > So.. what needs to be done there?
> > >
> > > > But, I just got an idea for how to handle this that might be halfway sane, maybe
> > > > I'll try and come up with a patch...
> > >
> > > Ok, here's such a patch, only lightly tested:
> >
> > I guess it would be nice for me to test it... but what it is against?
> > I tried after v4.10-rc5 and linux-next, but got rejects in both cases.
>
> Sorry, I forgot I had a few other patches in my branch that touch
> mempool/biosets code.
>
> Also, after thinking about it more and looking at the relevant code, I'm pretty
> sure we don't need rescuer threads for block devices that just split bios - i.e.
> most of them, so I changed my patch to do that.
>
> Tested it by ripping out the current->bio_list checks/workarounds from the
> bcache code, appears to work:
Feedback on this patch below, but first:
There are deeper issues with the current->bio_list and rescue workqueues
than thread counts.
I cannot help but feel like you (and Jens) are repeatedly ignoring the
issue that has been raised numerous times, most recently:
https://www.redhat.com/archives/dm-devel/2017-February/msg00059.html
FYI, this test (albeit ugly) can be used to check if the dm-snapshot
deadlock is fixed:
https://www.redhat.com/archives/dm-devel/2017-January/msg00064.html
This situation is the unfortunate pathological worst case for what
happens when changes are merged and nobody wants to own fixing the
unforseen implications/regressions. Like everyone else in a position
of Linux maintenance I've tried to stay away from owning the
responsibility of a fix -- it isn't working. Ok, I'll stop bitching
now.. I do bear responsibility for not digging in myself. We're all
busy and this issue is "hard".
> -- >8 --
> Subject: [PATCH] block: Make rescuer threads per request_queue, not per bioset
>
> Also, trigger rescuing whenever with bios on current->bio_list, instead
> of only when we block in bio_alloc_bioset(). This is more correct, and
> should result in fewer rescuer threads.
>
> XXX: The current->bio_list plugging needs to be unified with the
> blk_plug mechanism.
>
> Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
> ---
...
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 3086da5664..e1b22a68d9 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1490,7 +1490,7 @@ static struct mapped_device *alloc_dev(int minor)
> INIT_LIST_HEAD(&md->table_devices);
> spin_lock_init(&md->uevent_lock);
>
> - md->queue = blk_alloc_queue_node(GFP_KERNEL, numa_node_id);
> + md->queue = blk_alloc_queue_node(GFP_KERNEL, numa_node_id, 0);
> if (!md->queue)
> goto bad;
>
This should be BLK_QUEUE_NO_RESCUER as DM isn't making direct use of
bio_queue_split() for its own internal spliting (maybe it should and
that'd start to fix the issue I've been harping about?) but as is DM
destroys the rescuer workqueue (since commit dbba42d8a9eb "dm: eliminate
unused "bioset" process for each bio-based DM device").
Mike
^ permalink raw reply
* Re: [PATCH 0/4 v2] BDI lifetime fix
From: Dan Williams @ 2017-02-08 16:00 UTC (permalink / raw)
To: Jan Kara
Cc: Tejun Heo, Thiago Jung Bauermann, Jens Axboe, linux-block,
Christoph Hellwig, Laurent Dufour
In-Reply-To: <20170208102349.GA27541@quack2.suse.cz>
On Wed, Feb 8, 2017 at 2:23 AM, Jan Kara <jack@suse.cz> wrote:
> On Wed 08-02-17 08:51:42, Jan Kara wrote:
[..]
> <DETOUR BEGIN>
> This ordering seems to be the real culprit of the bdi name reuse problems
> Omar has reported? Same as described in commit 6cd18e711dd8 for MD BTW and
> Dan's patch could be IMHO replaced by a move of bdi_unregister() from
> blk_cleanup_queue() to del_gendisk() where it logically belongs as a
> counterpart of device_add_disk(). I'll test that.
> <DETOUR END>
I'd support that change. This is is why I called my fix "brute-force",
it just accommodated the existing problematic ordering.
^ permalink raw reply
* Re: [PATCH 08/24] btrfs: Convert to separately allocated bdi
From: David Sterba @ 2017-02-08 15:22 UTC (permalink / raw)
To: Jan Kara
Cc: linux-fsdevel, Christoph Hellwig, linux-block, Chris Mason,
Josef Bacik, David Sterba, linux-btrfs
In-Reply-To: <20170202173422.3240-9-jack@suse.cz>
On Thu, Feb 02, 2017 at 06:34:06PM +0100, Jan Kara wrote:
> Allocate struct backing_dev_info separately instead of embedding it
> inside superblock. This unifies handling of bdi among users.
>
> CC: Chris Mason <clm@fb.com>
> CC: Josef Bacik <jbacik@fb.com>
> CC: David Sterba <dsterba@suse.com>
> CC: linux-btrfs@vger.kernel.org
> Signed-off-by: Jan Kara <jack@suse.cz>
Reviewed-by: David Sterba <dsterba@suse.com>
^ permalink raw reply
* [PATCH V2] nbd: ref count the socks array
From: Josef Bacik @ 2017-02-08 15:05 UTC (permalink / raw)
To: linux-block, kernel-team, nbd-general, axboe
In-Reply-To: <1486505453-2976-1-git-send-email-jbacik@fb.com>
In preparation for allowing seamless reconnects we need a way to make
sure that we don't free the socks array out from underneath ourselves.
So a socks_ref counter in order to keep track of who is using the socks
array, and only free it and change num_connections once our reference
reduces to zero.
We also need to make sure that somebody calling SET_SOCK isn't coming in
before we're done with our socks array, so add a waitqueue to wait on
previous users of the socks array before initiating a new socks array.
Signed-off-by: Josef Bacik <jbacik@fb.com>
---
V1->V2:
-req->errors++ in the timeout handler if we can't get a ref on our socks.
-drop another use of nbd->config_lock in the timeout handler I missed.
drivers/block/nbd.c | 131 +++++++++++++++++++++++++++++++++++++---------------
1 file changed, 93 insertions(+), 38 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 1914ba2..afb1353 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -54,19 +54,24 @@ struct nbd_sock {
#define NBD_TIMEDOUT 0
#define NBD_DISCONNECT_REQUESTED 1
#define NBD_DISCONNECTED 2
-#define NBD_RUNNING 3
+#define NBD_HAS_SOCKS_REF 3
struct nbd_device {
u32 flags;
unsigned long runtime_flags;
+
+ struct mutex socks_lock;
struct nbd_sock **socks;
+ atomic_t socks_ref;
+ wait_queue_head_t socks_wq;
+ int num_connections;
+
int magic;
struct blk_mq_tag_set tag_set;
struct mutex config_lock;
struct gendisk *disk;
- int num_connections;
atomic_t recv_threads;
wait_queue_head_t recv_wq;
loff_t blksize;
@@ -102,7 +107,6 @@ static int part_shift;
static int nbd_dev_dbg_init(struct nbd_device *nbd);
static void nbd_dev_dbg_close(struct nbd_device *nbd);
-
static inline struct device *nbd_to_dev(struct nbd_device *nbd)
{
return disk_to_dev(nbd->disk);
@@ -125,6 +129,27 @@ static const char *nbdcmd_to_ascii(int cmd)
return "invalid";
}
+static int nbd_socks_get_unless_zero(struct nbd_device *nbd)
+{
+ return atomic_add_unless(&nbd->socks_ref, 1, 0);
+}
+
+static void nbd_socks_put(struct nbd_device *nbd)
+{
+ if (atomic_dec_and_test(&nbd->socks_ref)) {
+ mutex_lock(&nbd->socks_lock);
+ if (nbd->num_connections) {
+ int i;
+ for (i = 0; i < nbd->num_connections; i++)
+ kfree(nbd->socks[i]);
+ kfree(nbd->socks);
+ nbd->num_connections = 0;
+ nbd->socks = NULL;
+ }
+ mutex_unlock(&nbd->socks_lock);
+ }
+}
+
static int nbd_size_clear(struct nbd_device *nbd, struct block_device *bdev)
{
bdev->bd_inode->i_size = 0;
@@ -190,6 +215,7 @@ static void sock_shutdown(struct nbd_device *nbd)
mutex_lock(&nsock->tx_lock);
kernel_sock_shutdown(nsock->sock, SHUT_RDWR);
mutex_unlock(&nsock->tx_lock);
+ nsock->dead = true;
}
dev_warn(disk_to_dev(nbd->disk), "shutting down sockets\n");
}
@@ -200,10 +226,14 @@ static enum blk_eh_timer_return nbd_xmit_timeout(struct request *req,
struct nbd_cmd *cmd = blk_mq_rq_to_pdu(req);
struct nbd_device *nbd = cmd->nbd;
+ if (!nbd_socks_get_unless_zero(nbd)) {
+ req->errors++;
+ return BLK_EH_HANDLED;
+ }
+
if (nbd->num_connections > 1) {
dev_err_ratelimited(nbd_to_dev(nbd),
"Connection timed out, retrying\n");
- mutex_lock(&nbd->config_lock);
/*
* Hooray we have more connections, requeue this IO, the submit
* path will put it on a real connection.
@@ -217,21 +247,19 @@ static enum blk_eh_timer_return nbd_xmit_timeout(struct request *req,
kernel_sock_shutdown(nsock->sock, SHUT_RDWR);
mutex_unlock(&nsock->tx_lock);
}
- mutex_unlock(&nbd->config_lock);
blk_mq_requeue_request(req, true);
+ nbd_socks_put(nbd);
return BLK_EH_RESET_TIMER;
}
- mutex_unlock(&nbd->config_lock);
} else {
dev_err_ratelimited(nbd_to_dev(nbd),
"Connection timed out\n");
}
set_bit(NBD_TIMEDOUT, &nbd->runtime_flags);
req->errors++;
-
- mutex_lock(&nbd->config_lock);
sock_shutdown(nbd);
- mutex_unlock(&nbd->config_lock);
+ nbd_socks_put(nbd);
+
return BLK_EH_HANDLED;
}
@@ -523,6 +551,7 @@ static void recv_work(struct work_struct *work)
nbd_end_request(cmd);
}
+ nbd_socks_put(nbd);
atomic_dec(&nbd->recv_threads);
wake_up(&nbd->recv_wq);
}
@@ -598,9 +627,16 @@ static int nbd_handle_cmd(struct nbd_cmd *cmd, int index)
struct nbd_sock *nsock;
int ret;
+ if (!nbd_socks_get_unless_zero(nbd)) {
+ dev_err_ratelimited(disk_to_dev(nbd->disk),
+ "Socks array is empty\n");
+ return -EINVAL;
+ }
+
if (index >= nbd->num_connections) {
dev_err_ratelimited(disk_to_dev(nbd->disk),
"Attempted send on invalid socket\n");
+ nbd_socks_put(nbd);
return -EINVAL;
}
req->errors = 0;
@@ -608,8 +644,10 @@ static int nbd_handle_cmd(struct nbd_cmd *cmd, int index)
nsock = nbd->socks[index];
if (nsock->dead) {
index = find_fallback(nbd, index);
- if (index < 0)
+ if (index < 0) {
+ nbd_socks_put(nbd);
return -EIO;
+ }
nsock = nbd->socks[index];
}
@@ -627,7 +665,7 @@ static int nbd_handle_cmd(struct nbd_cmd *cmd, int index)
goto again;
}
mutex_unlock(&nsock->tx_lock);
-
+ nbd_socks_put(nbd);
return ret;
}
@@ -656,6 +694,25 @@ static int nbd_queue_rq(struct blk_mq_hw_ctx *hctx,
return BLK_MQ_RQ_QUEUE_OK;
}
+static int nbd_wait_for_socks(struct nbd_device *nbd)
+{
+ int ret;
+
+ if (!atomic_read(&nbd->socks_ref))
+ return 0;
+
+ do {
+ mutex_unlock(&nbd->socks_lock);
+ mutex_unlock(&nbd->config_lock);
+ ret = wait_event_interruptible(nbd->socks_wq,
+ atomic_read(&nbd->socks_ref) == 0);
+ mutex_lock(&nbd->config_lock);
+ mutex_lock(&nbd->socks_lock);
+ } while (!ret && atomic_read(&nbd->socks_ref));
+
+ return ret;
+}
+
static int nbd_add_socket(struct nbd_device *nbd, struct block_device *bdev,
unsigned long arg)
{
@@ -668,21 +725,30 @@ static int nbd_add_socket(struct nbd_device *nbd, struct block_device *bdev,
if (!sock)
return err;
- if (!nbd->task_setup)
+ err = -EINVAL;
+ mutex_lock(&nbd->socks_lock);
+ if (!nbd->task_setup) {
nbd->task_setup = current;
+ if (nbd_wait_for_socks(nbd))
+ goto out;
+ atomic_inc(&nbd->socks_ref);
+ set_bit(NBD_HAS_SOCKS_REF, &nbd->runtime_flags);
+ }
+
if (nbd->task_setup != current) {
dev_err(disk_to_dev(nbd->disk),
"Device being setup by another task");
- return -EINVAL;
+ goto out;
}
+ err = -ENOMEM;
socks = krealloc(nbd->socks, (nbd->num_connections + 1) *
sizeof(struct nbd_sock *), GFP_KERNEL);
if (!socks)
- return -ENOMEM;
+ goto out;
nsock = kzalloc(sizeof(struct nbd_sock), GFP_KERNEL);
if (!nsock)
- return -ENOMEM;
+ goto out;
nbd->socks = socks;
@@ -694,7 +760,10 @@ static int nbd_add_socket(struct nbd_device *nbd, struct block_device *bdev,
if (max_part)
bdev->bd_invalidated = 1;
- return 0;
+ err = 0;
+out:
+ mutex_unlock(&nbd->socks_lock);
+ return err;
}
/* Reset all properties of an NBD device */
@@ -750,20 +819,17 @@ static void send_disconnects(struct nbd_device *nbd)
static int nbd_disconnect(struct nbd_device *nbd, struct block_device *bdev)
{
dev_info(disk_to_dev(nbd->disk), "NBD_DISCONNECT\n");
- if (!nbd->socks)
+ if (!nbd_socks_get_unless_zero(nbd))
return -EINVAL;
mutex_unlock(&nbd->config_lock);
fsync_bdev(bdev);
mutex_lock(&nbd->config_lock);
- /* Check again after getting mutex back. */
- if (!nbd->socks)
- return -EINVAL;
-
if (!test_and_set_bit(NBD_DISCONNECT_REQUESTED,
&nbd->runtime_flags))
send_disconnects(nbd);
+ nbd_socks_put(nbd);
return 0;
}
@@ -773,22 +839,9 @@ static int nbd_clear_sock(struct nbd_device *nbd, struct block_device *bdev)
nbd_clear_que(nbd);
kill_bdev(bdev);
nbd_bdev_reset(bdev);
- /*
- * We want to give the run thread a chance to wait for everybody
- * to clean up and then do it's own cleanup.
- */
- if (!test_bit(NBD_RUNNING, &nbd->runtime_flags) &&
- nbd->num_connections) {
- int i;
-
- for (i = 0; i < nbd->num_connections; i++)
- kfree(nbd->socks[i]);
- kfree(nbd->socks);
- nbd->socks = NULL;
- nbd->num_connections = 0;
- }
nbd->task_setup = NULL;
-
+ if (test_and_clear_bit(NBD_HAS_SOCKS_REF, &nbd->runtime_flags))
+ nbd_socks_put(nbd);
return 0;
}
@@ -809,7 +862,6 @@ static int nbd_start_device(struct nbd_device *nbd, struct block_device *bdev)
goto out_err;
}
- set_bit(NBD_RUNNING, &nbd->runtime_flags);
blk_mq_update_nr_hw_queues(&nbd->tag_set, nbd->num_connections);
args = kcalloc(num_connections, sizeof(*args), GFP_KERNEL);
if (!args) {
@@ -833,6 +885,7 @@ static int nbd_start_device(struct nbd_device *nbd, struct block_device *bdev)
for (i = 0; i < num_connections; i++) {
sk_set_memalloc(nbd->socks[i]->sock->sk);
atomic_inc(&nbd->recv_threads);
+ atomic_inc(&nbd->socks_ref);
INIT_WORK(&args[i].work, recv_work);
args[i].nbd = nbd;
args[i].index = i;
@@ -849,7 +902,6 @@ static int nbd_start_device(struct nbd_device *nbd, struct block_device *bdev)
mutex_lock(&nbd->config_lock);
nbd->task_recv = NULL;
out_err:
- clear_bit(NBD_RUNNING, &nbd->runtime_flags);
nbd_clear_sock(nbd, bdev);
/* user requested, ignore socket errors */
@@ -1149,12 +1201,15 @@ static int nbd_dev_add(int index)
nbd->magic = NBD_MAGIC;
mutex_init(&nbd->config_lock);
+ mutex_init(&nbd->socks_lock);
+ atomic_set(&nbd->socks_ref, 0);
disk->major = NBD_MAJOR;
disk->first_minor = index << part_shift;
disk->fops = &nbd_fops;
disk->private_data = nbd;
sprintf(disk->disk_name, "nbd%d", index);
init_waitqueue_head(&nbd->recv_wq);
+ init_waitqueue_head(&nbd->socks_wq);
nbd_reset(nbd);
add_disk(disk);
return index;
--
2.7.4
^ permalink raw reply related
* [PATCH 4/4] nvme: support ranged discard requests
From: Christoph Hellwig @ 2017-02-08 13:46 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, linux-nvme
In-Reply-To: <20170208134650.5380-1-hch@lst.de>
NVMe supports up to 256 ranges per DSM command, so wire up support
for ranged discards up to that limit.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/nvme/host/core.c | 30 +++++++++++++++++++++++-------
include/linux/nvme.h | 2 ++
2 files changed, 25 insertions(+), 7 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index efe8ec300126..d9f49036819c 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -238,26 +238,38 @@ static inline void nvme_setup_flush(struct nvme_ns *ns,
static inline int nvme_setup_discard(struct nvme_ns *ns, struct request *req,
struct nvme_command *cmnd)
{
+ unsigned short segments = blk_rq_nr_discard_segments(req), n = 0;
struct nvme_dsm_range *range;
- unsigned int nr_bytes = blk_rq_bytes(req);
+ struct bio *bio;
- range = kmalloc(sizeof(*range), GFP_ATOMIC);
+ range = kmalloc_array(segments, sizeof(*range), GFP_ATOMIC);
if (!range)
return BLK_MQ_RQ_QUEUE_BUSY;
- range->cattr = cpu_to_le32(0);
- range->nlb = cpu_to_le32(nr_bytes >> ns->lba_shift);
- range->slba = cpu_to_le64(nvme_block_nr(ns, blk_rq_pos(req)));
+ __rq_for_each_bio(bio, req) {
+ u64 slba = nvme_block_nr(ns, bio->bi_iter.bi_sector);
+ u32 nlb = bio->bi_iter.bi_size >> ns->lba_shift;
+
+ range[n].cattr = cpu_to_le32(0);
+ range[n].nlb = cpu_to_le32(nlb);
+ range[n].slba = cpu_to_le64(slba);
+ n++;
+ }
+
+ if (WARN_ON_ONCE(n != segments)) {
+ kfree(range);
+ return BLK_MQ_RQ_QUEUE_ERROR;
+ }
memset(cmnd, 0, sizeof(*cmnd));
cmnd->dsm.opcode = nvme_cmd_dsm;
cmnd->dsm.nsid = cpu_to_le32(ns->ns_id);
- cmnd->dsm.nr = 0;
+ cmnd->dsm.nr = segments - 1;
cmnd->dsm.attributes = cpu_to_le32(NVME_DSMGMT_AD);
req->special_vec.bv_page = virt_to_page(range);
req->special_vec.bv_offset = offset_in_page(range);
- req->special_vec.bv_len = sizeof(*range);
+ req->special_vec.bv_len = sizeof(*range) * segments;
req->rq_flags |= RQF_SPECIAL_PAYLOAD;
return BLK_MQ_RQ_QUEUE_OK;
@@ -877,6 +889,9 @@ static void nvme_config_discard(struct nvme_ns *ns)
struct nvme_ctrl *ctrl = ns->ctrl;
u32 logical_block_size = queue_logical_block_size(ns->queue);
+ BUILD_BUG_ON(PAGE_SIZE / sizeof(struct nvme_dsm_range) <
+ NVME_DSM_MAX_RANGES);
+
if (ctrl->quirks & NVME_QUIRK_DISCARD_ZEROES)
ns->queue->limits.discard_zeroes_data = 1;
else
@@ -885,6 +900,7 @@ static void nvme_config_discard(struct nvme_ns *ns)
ns->queue->limits.discard_alignment = logical_block_size;
ns->queue->limits.discard_granularity = logical_block_size;
blk_queue_max_discard_sectors(ns->queue, UINT_MAX);
+ blk_queue_max_discard_segments(ns->queue, NVME_DSM_MAX_RANGES);
queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, ns->queue);
}
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 3d1c6f1b15c9..3e2ed49c3ad8 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -553,6 +553,8 @@ enum {
NVME_DSMGMT_AD = 1 << 2,
};
+#define NVME_DSM_MAX_RANGES 256
+
struct nvme_dsm_range {
__le32 cattr;
__le32 nlb;
--
2.11.0
^ permalink raw reply related
* [PATCH 3/4] block: optionally merge discontiguous discard bios into a single request
From: Christoph Hellwig @ 2017-02-08 13:46 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, linux-nvme
In-Reply-To: <20170208134650.5380-1-hch@lst.de>
Add a new merge strategy that merges discard bios into a request until the
maximum number of discard ranges (or the maximum discard size) is reached
from the plug merging code. I/O scheduler merging is not wired up yet
but might also be useful, although not for fast devices like NVMe which
are the only user for now.
Note that for now we don't support limiting the size of each discard range,
but if needed that can be added later.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/blk-core.c | 27 +++++++++++++++++++++++++++
block/blk-merge.c | 5 ++++-
block/blk-mq.c | 3 +++
block/blk-settings.c | 20 ++++++++++++++++++++
block/blk-sysfs.c | 12 ++++++++++++
block/blk.h | 2 ++
include/linux/blkdev.h | 26 ++++++++++++++++++++++++++
include/linux/elevator.h | 1 +
8 files changed, 95 insertions(+), 1 deletion(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index 75fe534861df..b9e857f4afe8 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1485,6 +1485,30 @@ bool bio_attempt_front_merge(struct request_queue *q, struct request *req,
return true;
}
+bool bio_attempt_discard_merge(struct request_queue *q, struct request *req,
+ struct bio *bio)
+{
+ unsigned short segments = blk_rq_nr_discard_segments(req);
+
+ if (segments >= queue_max_discard_segments(q))
+ goto no_merge;
+ if (blk_rq_sectors(req) + bio_sectors(bio) >
+ blk_rq_get_max_sectors(req, blk_rq_pos(req)))
+ goto no_merge;
+
+ req->biotail->bi_next = bio;
+ req->biotail = bio;
+ req->__data_len += bio->bi_iter.bi_size;
+ req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
+ req->nr_phys_segments = segments + 1;
+
+ blk_account_io_start(req, false);
+ return true;
+no_merge:
+ req_set_nomerge(q, req);
+ return false;
+}
+
/**
* blk_attempt_plug_merge - try to merge with %current's plugged list
* @q: request_queue new bio is being queued at
@@ -1549,6 +1573,9 @@ bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio,
case ELEVATOR_FRONT_MERGE:
merged = bio_attempt_front_merge(q, rq, bio);
break;
+ case ELEVATOR_DISCARD_MERGE:
+ merged = bio_attempt_discard_merge(q, rq, bio);
+ break;
default:
break;
}
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 6cbd90ad5f90..2afa262425d1 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -803,7 +803,10 @@ bool blk_rq_merge_ok(struct request *rq, struct bio *bio)
enum elv_merge blk_try_merge(struct request *rq, struct bio *bio)
{
- if (blk_rq_pos(rq) + blk_rq_sectors(rq) == bio->bi_iter.bi_sector)
+ if (req_op(rq) == REQ_OP_DISCARD &&
+ queue_max_discard_segments(rq->q) > 1)
+ return ELEVATOR_DISCARD_MERGE;
+ else if (blk_rq_pos(rq) + blk_rq_sectors(rq) == bio->bi_iter.bi_sector)
return ELEVATOR_BACK_MERGE;
else if (blk_rq_pos(rq) - bio_sectors(bio) == bio->bi_iter.bi_sector)
return ELEVATOR_FRONT_MERGE;
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 9294633759d9..89cb2d224488 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -780,6 +780,9 @@ static bool blk_mq_attempt_merge(struct request_queue *q,
if (blk_mq_sched_allow_merge(q, rq, bio))
merged = bio_attempt_front_merge(q, rq, bio);
break;
+ case ELEVATOR_DISCARD_MERGE:
+ merged = bio_attempt_discard_merge(q, rq, bio);
+ break;
default:
continue;
}
diff --git a/block/blk-settings.c b/block/blk-settings.c
index 6eb19bcbf3cb..1e7174ffc9d4 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -88,6 +88,7 @@ EXPORT_SYMBOL_GPL(blk_queue_lld_busy);
void blk_set_default_limits(struct queue_limits *lim)
{
lim->max_segments = BLK_MAX_SEGMENTS;
+ lim->max_discard_segments = 1;
lim->max_integrity_segments = 0;
lim->seg_boundary_mask = BLK_SEG_BOUNDARY_MASK;
lim->virt_boundary_mask = 0;
@@ -128,6 +129,7 @@ void blk_set_stacking_limits(struct queue_limits *lim)
/* Inherit limits from component devices */
lim->discard_zeroes_data = 1;
lim->max_segments = USHRT_MAX;
+ lim->max_discard_segments = 1;
lim->max_hw_sectors = UINT_MAX;
lim->max_segment_size = UINT_MAX;
lim->max_sectors = UINT_MAX;
@@ -337,6 +339,22 @@ void blk_queue_max_segments(struct request_queue *q, unsigned short max_segments
EXPORT_SYMBOL(blk_queue_max_segments);
/**
+ * blk_queue_max_discard_segments - set max segments for discard requests
+ * @q: the request queue for the device
+ * @max_segments: max number of segments
+ *
+ * Description:
+ * Enables a low level driver to set an upper limit on the number of
+ * segments in a discard request.
+ **/
+void blk_queue_max_discard_segments(struct request_queue *q,
+ unsigned short max_segments)
+{
+ q->limits.max_discard_segments = max_segments;
+}
+EXPORT_SYMBOL_GPL(blk_queue_max_discard_segments);
+
+/**
* blk_queue_max_segment_size - set max segment size for blk_rq_map_sg
* @q: the request queue for the device
* @max_size: max size of segment in bytes
@@ -553,6 +571,8 @@ int blk_stack_limits(struct queue_limits *t, struct queue_limits *b,
b->virt_boundary_mask);
t->max_segments = min_not_zero(t->max_segments, b->max_segments);
+ t->max_discard_segments = min_not_zero(t->max_discard_segments,
+ b->max_discard_segments);
t->max_integrity_segments = min_not_zero(t->max_integrity_segments,
b->max_integrity_segments);
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 48032c4759a7..070d81bae1d5 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -121,6 +121,12 @@ static ssize_t queue_max_segments_show(struct request_queue *q, char *page)
return queue_var_show(queue_max_segments(q), (page));
}
+static ssize_t queue_max_discard_segments_show(struct request_queue *q,
+ char *page)
+{
+ return queue_var_show(queue_max_discard_segments(q), (page));
+}
+
static ssize_t queue_max_integrity_segments_show(struct request_queue *q, char *page)
{
return queue_var_show(q->limits.max_integrity_segments, (page));
@@ -545,6 +551,11 @@ static struct queue_sysfs_entry queue_max_segments_entry = {
.show = queue_max_segments_show,
};
+static struct queue_sysfs_entry queue_max_discard_segments_entry = {
+ .attr = {.name = "max_discard_segments", .mode = S_IRUGO },
+ .show = queue_max_discard_segments_show,
+};
+
static struct queue_sysfs_entry queue_max_integrity_segments_entry = {
.attr = {.name = "max_integrity_segments", .mode = S_IRUGO },
.show = queue_max_integrity_segments_show,
@@ -697,6 +708,7 @@ static struct attribute *default_attrs[] = {
&queue_max_hw_sectors_entry.attr,
&queue_max_sectors_entry.attr,
&queue_max_segments_entry.attr,
+ &queue_max_discard_segments_entry.attr,
&queue_max_integrity_segments_entry.attr,
&queue_max_segment_size_entry.attr,
&queue_iosched_entry.attr,
diff --git a/block/blk.h b/block/blk.h
index ae82f2ac4019..d1ea4bd9b9a3 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -100,6 +100,8 @@ bool bio_attempt_front_merge(struct request_queue *q, struct request *req,
struct bio *bio);
bool bio_attempt_back_merge(struct request_queue *q, struct request *req,
struct bio *bio);
+bool bio_attempt_discard_merge(struct request_queue *q, struct request *req,
+ struct bio *bio);
bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio,
unsigned int *request_count,
struct request **same_queue_rq);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index e0bac14347e6..aecca0e7d9ca 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -331,6 +331,7 @@ struct queue_limits {
unsigned short logical_block_size;
unsigned short max_segments;
unsigned short max_integrity_segments;
+ unsigned short max_discard_segments;
unsigned char misaligned;
unsigned char discard_misaligned;
@@ -1146,6 +1147,8 @@ extern void blk_queue_bounce_limit(struct request_queue *, u64);
extern void blk_queue_max_hw_sectors(struct request_queue *, unsigned int);
extern void blk_queue_chunk_sectors(struct request_queue *, unsigned int);
extern void blk_queue_max_segments(struct request_queue *, unsigned short);
+extern void blk_queue_max_discard_segments(struct request_queue *,
+ unsigned short);
extern void blk_queue_max_segment_size(struct request_queue *, unsigned int);
extern void blk_queue_max_discard_sectors(struct request_queue *q,
unsigned int max_discard_sectors);
@@ -1189,6 +1192,15 @@ extern void blk_queue_rq_timeout(struct request_queue *, unsigned int);
extern void blk_queue_flush_queueable(struct request_queue *q, bool queueable);
extern void blk_queue_write_cache(struct request_queue *q, bool enabled, bool fua);
+/*
+ * Number of physical segments as sent to the device.
+ *
+ * Normally this is the number of discontiguous data segments sent by the
+ * submitter. But for data-less command like discard we might have no
+ * actual data segments submitted, but the driver might have to add it's
+ * own special payload. In that case we still return 1 here so that this
+ * special payload will be mapped.
+ */
static inline unsigned short blk_rq_nr_phys_segments(struct request *rq)
{
if (rq->rq_flags & RQF_SPECIAL_PAYLOAD)
@@ -1196,6 +1208,15 @@ static inline unsigned short blk_rq_nr_phys_segments(struct request *rq)
return rq->nr_phys_segments;
}
+/*
+ * Number of discard segments (or ranges) the driver needs to fill in.
+ * Each discard bio merged into a request is counted as one segment.
+ */
+static inline unsigned short blk_rq_nr_discard_segments(struct request *rq)
+{
+ return max_t(unsigned short, rq->nr_phys_segments, 1);
+}
+
extern int blk_rq_map_sg(struct request_queue *, struct request *, struct scatterlist *);
extern void blk_dump_rq_flags(struct request *, char *);
extern long nr_blockdev_pages(void);
@@ -1384,6 +1405,11 @@ static inline unsigned short queue_max_segments(struct request_queue *q)
return q->limits.max_segments;
}
+static inline unsigned short queue_max_discard_segments(struct request_queue *q)
+{
+ return q->limits.max_discard_segments;
+}
+
static inline unsigned int queue_max_segment_size(struct request_queue *q)
{
return q->limits.max_segment_size;
diff --git a/include/linux/elevator.h b/include/linux/elevator.h
index b38b4e651ea6..8265b6330cc2 100644
--- a/include/linux/elevator.h
+++ b/include/linux/elevator.h
@@ -16,6 +16,7 @@ enum elv_merge {
ELEVATOR_NO_MERGE = 0,
ELEVATOR_FRONT_MERGE = 1,
ELEVATOR_BACK_MERGE = 2,
+ ELEVATOR_DISCARD_MERGE = 3,
};
typedef enum elv_merge (elevator_merge_fn) (struct request_queue *, struct request **,
--
2.11.0
^ permalink raw reply related
* [PATCH 2/4] block: enumify ELEVATOR_*_MERGE
From: Christoph Hellwig @ 2017-02-08 13:46 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, linux-nvme
In-Reply-To: <20170208134650.5380-1-hch@lst.de>
Switch these constants to an enum, and make let the compiler ensure that
all callers of blk_try_merge and elv_merge handle all potential values.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/blk-core.c | 76 +++++++++++++++++++++++++-----------------------
block/blk-merge.c | 2 +-
block/blk-mq-sched.c | 35 +++++++++++-----------
block/blk-mq.c | 32 +++++++++-----------
block/blk.h | 2 +-
block/cfq-iosched.c | 4 +--
block/deadline-iosched.c | 12 +++-----
block/elevator.c | 10 ++++---
block/mq-deadline.c | 2 +-
include/linux/elevator.h | 28 ++++++++++--------
10 files changed, 102 insertions(+), 101 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index d161d4ab7052..75fe534861df 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1513,12 +1513,11 @@ bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio,
{
struct blk_plug *plug;
struct request *rq;
- bool ret = false;
struct list_head *plug_list;
plug = current->plug;
if (!plug)
- goto out;
+ return false;
*request_count = 0;
if (q->mq_ops)
@@ -1527,7 +1526,7 @@ bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio,
plug_list = &plug->list;
list_for_each_entry_reverse(rq, plug_list, queuelist) {
- int el_ret;
+ bool merged = false;
if (rq->q == q) {
(*request_count)++;
@@ -1543,19 +1542,22 @@ bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio,
if (rq->q != q || !blk_rq_merge_ok(rq, bio))
continue;
- el_ret = blk_try_merge(rq, bio);
- if (el_ret == ELEVATOR_BACK_MERGE) {
- ret = bio_attempt_back_merge(q, rq, bio);
- if (ret)
- break;
- } else if (el_ret == ELEVATOR_FRONT_MERGE) {
- ret = bio_attempt_front_merge(q, rq, bio);
- if (ret)
- break;
+ switch (blk_try_merge(rq, bio)) {
+ case ELEVATOR_BACK_MERGE:
+ merged = bio_attempt_back_merge(q, rq, bio);
+ break;
+ case ELEVATOR_FRONT_MERGE:
+ merged = bio_attempt_front_merge(q, rq, bio);
+ break;
+ default:
+ break;
}
+
+ if (merged)
+ return true;
}
-out:
- return ret;
+
+ return false;
}
unsigned int blk_plug_queued_count(struct request_queue *q)
@@ -1597,7 +1599,7 @@ void init_request_from_bio(struct request *req, struct bio *bio)
static blk_qc_t blk_queue_bio(struct request_queue *q, struct bio *bio)
{
struct blk_plug *plug;
- int el_ret, where = ELEVATOR_INSERT_SORT;
+ int where = ELEVATOR_INSERT_SORT;
struct request *req, *free;
unsigned int request_count = 0;
unsigned int wb_acct;
@@ -1635,27 +1637,29 @@ static blk_qc_t blk_queue_bio(struct request_queue *q, struct bio *bio)
spin_lock_irq(q->queue_lock);
- el_ret = elv_merge(q, &req, bio);
- if (el_ret == ELEVATOR_BACK_MERGE) {
- if (bio_attempt_back_merge(q, req, bio)) {
- elv_bio_merged(q, req, bio);
- free = attempt_back_merge(q, req);
- if (!free)
- elv_merged_request(q, req, el_ret);
- else
- __blk_put_request(q, free);
- goto out_unlock;
- }
- } else if (el_ret == ELEVATOR_FRONT_MERGE) {
- if (bio_attempt_front_merge(q, req, bio)) {
- elv_bio_merged(q, req, bio);
- free = attempt_front_merge(q, req);
- if (!free)
- elv_merged_request(q, req, el_ret);
- else
- __blk_put_request(q, free);
- goto out_unlock;
- }
+ switch (elv_merge(q, &req, bio)) {
+ case ELEVATOR_BACK_MERGE:
+ if (!bio_attempt_back_merge(q, req, bio))
+ break;
+ elv_bio_merged(q, req, bio);
+ free = attempt_back_merge(q, req);
+ if (free)
+ __blk_put_request(q, free);
+ else
+ elv_merged_request(q, req, ELEVATOR_BACK_MERGE);
+ goto out_unlock;
+ case ELEVATOR_FRONT_MERGE:
+ if (!bio_attempt_front_merge(q, req, bio))
+ break;
+ elv_bio_merged(q, req, bio);
+ free = attempt_front_merge(q, req);
+ if (free)
+ __blk_put_request(q, free);
+ else
+ elv_merged_request(q, req, ELEVATOR_FRONT_MERGE);
+ goto out_unlock;
+ default:
+ break;
}
get_rq:
diff --git a/block/blk-merge.c b/block/blk-merge.c
index c956d9e7aafd..6cbd90ad5f90 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -801,7 +801,7 @@ bool blk_rq_merge_ok(struct request *rq, struct bio *bio)
return true;
}
-int blk_try_merge(struct request *rq, struct bio *bio)
+enum elv_merge blk_try_merge(struct request *rq, struct bio *bio)
{
if (blk_rq_pos(rq) + blk_rq_sectors(rq) == bio->bi_iter.bi_sector)
return ELEVATOR_BACK_MERGE;
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index ee455e7cf9d8..72d0d8361175 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -238,30 +238,29 @@ bool blk_mq_sched_try_merge(struct request_queue *q, struct bio *bio,
struct request **merged_request)
{
struct request *rq;
- int ret;
- ret = elv_merge(q, &rq, bio);
- if (ret == ELEVATOR_BACK_MERGE) {
+ switch (elv_merge(q, &rq, bio)) {
+ case ELEVATOR_BACK_MERGE:
if (!blk_mq_sched_allow_merge(q, rq, bio))
return false;
- if (bio_attempt_back_merge(q, rq, bio)) {
- *merged_request = attempt_back_merge(q, rq);
- if (!*merged_request)
- elv_merged_request(q, rq, ret);
- return true;
- }
- } else if (ret == ELEVATOR_FRONT_MERGE) {
+ if (!bio_attempt_back_merge(q, rq, bio))
+ return false;
+ *merged_request = attempt_back_merge(q, rq);
+ if (!*merged_request)
+ elv_merged_request(q, rq, ELEVATOR_BACK_MERGE);
+ return true;
+ case ELEVATOR_FRONT_MERGE:
if (!blk_mq_sched_allow_merge(q, rq, bio))
return false;
- if (bio_attempt_front_merge(q, rq, bio)) {
- *merged_request = attempt_front_merge(q, rq);
- if (!*merged_request)
- elv_merged_request(q, rq, ret);
- return true;
- }
+ if (!bio_attempt_front_merge(q, rq, bio))
+ return false;
+ *merged_request = attempt_front_merge(q, rq);
+ if (!*merged_request)
+ elv_merged_request(q, rq, ELEVATOR_FRONT_MERGE);
+ return true;
+ default:
+ return false;
}
-
- return false;
}
EXPORT_SYMBOL_GPL(blk_mq_sched_try_merge);
diff --git a/block/blk-mq.c b/block/blk-mq.c
index be183e6115a1..9294633759d9 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -763,7 +763,7 @@ static bool blk_mq_attempt_merge(struct request_queue *q,
int checked = 8;
list_for_each_entry_reverse(rq, &ctx->rq_list, queuelist) {
- int el_ret;
+ bool merged = false;
if (!checked--)
break;
@@ -771,26 +771,22 @@ static bool blk_mq_attempt_merge(struct request_queue *q,
if (!blk_rq_merge_ok(rq, bio))
continue;
- el_ret = blk_try_merge(rq, bio);
- if (el_ret == ELEVATOR_NO_MERGE)
- continue;
-
- if (!blk_mq_sched_allow_merge(q, rq, bio))
+ switch (blk_try_merge(rq, bio)) {
+ case ELEVATOR_BACK_MERGE:
+ if (blk_mq_sched_allow_merge(q, rq, bio))
+ merged = bio_attempt_back_merge(q, rq, bio);
break;
-
- if (el_ret == ELEVATOR_BACK_MERGE) {
- if (bio_attempt_back_merge(q, rq, bio)) {
- ctx->rq_merged++;
- return true;
- }
- break;
- } else if (el_ret == ELEVATOR_FRONT_MERGE) {
- if (bio_attempt_front_merge(q, rq, bio)) {
- ctx->rq_merged++;
- return true;
- }
+ case ELEVATOR_FRONT_MERGE:
+ if (blk_mq_sched_allow_merge(q, rq, bio))
+ merged = bio_attempt_front_merge(q, rq, bio);
break;
+ default:
+ continue;
}
+
+ if (merged)
+ ctx->rq_merged++;
+ return merged;
}
return false;
diff --git a/block/blk.h b/block/blk.h
index 3e08703902a9..ae82f2ac4019 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -215,7 +215,7 @@ int blk_attempt_req_merge(struct request_queue *q, struct request *rq,
void blk_recalc_rq_segments(struct request *rq);
void blk_rq_set_mixed_merge(struct request *rq);
bool blk_rq_merge_ok(struct request *rq, struct bio *bio);
-int blk_try_merge(struct request *rq, struct bio *bio);
+enum elv_merge blk_try_merge(struct request *rq, struct bio *bio);
void blk_queue_congestion_threshold(struct request_queue *q);
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index f0f29ee731e1..921262770636 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -2528,7 +2528,7 @@ static void cfq_remove_request(struct request *rq)
}
}
-static int cfq_merge(struct request_queue *q, struct request **req,
+static enum elv_merge cfq_merge(struct request_queue *q, struct request **req,
struct bio *bio)
{
struct cfq_data *cfqd = q->elevator->elevator_data;
@@ -2544,7 +2544,7 @@ static int cfq_merge(struct request_queue *q, struct request **req,
}
static void cfq_merged_request(struct request_queue *q, struct request *req,
- int type)
+ enum elv_merge type)
{
if (type == ELEVATOR_FRONT_MERGE) {
struct cfq_queue *cfqq = RQ_CFQQ(req);
diff --git a/block/deadline-iosched.c b/block/deadline-iosched.c
index 05fc0ea25a98..c68f6bbc0dcd 100644
--- a/block/deadline-iosched.c
+++ b/block/deadline-iosched.c
@@ -120,12 +120,11 @@ static void deadline_remove_request(struct request_queue *q, struct request *rq)
deadline_del_rq_rb(dd, rq);
}
-static int
+static enum elv_merge
deadline_merge(struct request_queue *q, struct request **req, struct bio *bio)
{
struct deadline_data *dd = q->elevator->elevator_data;
struct request *__rq;
- int ret;
/*
* check for front merge
@@ -138,20 +137,17 @@ deadline_merge(struct request_queue *q, struct request **req, struct bio *bio)
BUG_ON(sector != blk_rq_pos(__rq));
if (elv_bio_merge_ok(__rq, bio)) {
- ret = ELEVATOR_FRONT_MERGE;
- goto out;
+ *req = __rq;
+ return ELEVATOR_FRONT_MERGE;
}
}
}
return ELEVATOR_NO_MERGE;
-out:
- *req = __rq;
- return ret;
}
static void deadline_merged_request(struct request_queue *q,
- struct request *req, int type)
+ struct request *req, enum elv_merge type)
{
struct deadline_data *dd = q->elevator->elevator_data;
diff --git a/block/elevator.c b/block/elevator.c
index 7e4f5880dd64..27ff1ed5a6fa 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -427,11 +427,11 @@ void elv_dispatch_add_tail(struct request_queue *q, struct request *rq)
}
EXPORT_SYMBOL(elv_dispatch_add_tail);
-int elv_merge(struct request_queue *q, struct request **req, struct bio *bio)
+enum elv_merge elv_merge(struct request_queue *q, struct request **req,
+ struct bio *bio)
{
struct elevator_queue *e = q->elevator;
struct request *__rq;
- int ret;
/*
* Levels of merges:
@@ -446,7 +446,8 @@ int elv_merge(struct request_queue *q, struct request **req, struct bio *bio)
* First try one-hit cache.
*/
if (q->last_merge && elv_bio_merge_ok(q->last_merge, bio)) {
- ret = blk_try_merge(q->last_merge, bio);
+ enum elv_merge ret = blk_try_merge(q->last_merge, bio);
+
if (ret != ELEVATOR_NO_MERGE) {
*req = q->last_merge;
return ret;
@@ -514,7 +515,8 @@ bool elv_attempt_insert_merge(struct request_queue *q, struct request *rq)
return ret;
}
-void elv_merged_request(struct request_queue *q, struct request *rq, int type)
+void elv_merged_request(struct request_queue *q, struct request *rq,
+ enum elv_merge type)
{
struct elevator_queue *e = q->elevator;
diff --git a/block/mq-deadline.c b/block/mq-deadline.c
index d68d9c273a66..236121633ca0 100644
--- a/block/mq-deadline.c
+++ b/block/mq-deadline.c
@@ -121,7 +121,7 @@ static void deadline_remove_request(struct request_queue *q, struct request *rq)
}
static void dd_request_merged(struct request_queue *q, struct request *req,
- int type)
+ enum elv_merge type)
{
struct deadline_data *dd = q->elevator->elevator_data;
diff --git a/include/linux/elevator.h b/include/linux/elevator.h
index b5825c4f06f7..b38b4e651ea6 100644
--- a/include/linux/elevator.h
+++ b/include/linux/elevator.h
@@ -9,12 +9,21 @@
struct io_cq;
struct elevator_type;
-typedef int (elevator_merge_fn) (struct request_queue *, struct request **,
+/*
+ * Return values from elevator merger
+ */
+enum elv_merge {
+ ELEVATOR_NO_MERGE = 0,
+ ELEVATOR_FRONT_MERGE = 1,
+ ELEVATOR_BACK_MERGE = 2,
+};
+
+typedef enum elv_merge (elevator_merge_fn) (struct request_queue *, struct request **,
struct bio *);
typedef void (elevator_merge_req_fn) (struct request_queue *, struct request *, struct request *);
-typedef void (elevator_merged_fn) (struct request_queue *, struct request *, int);
+typedef void (elevator_merged_fn) (struct request_queue *, struct request *, enum elv_merge);
typedef int (elevator_allow_bio_merge_fn) (struct request_queue *,
struct request *, struct bio *);
@@ -87,7 +96,7 @@ struct elevator_mq_ops {
bool (*allow_merge)(struct request_queue *, struct request *, struct bio *);
bool (*bio_merge)(struct blk_mq_hw_ctx *, struct bio *);
int (*request_merge)(struct request_queue *q, struct request **, struct bio *);
- void (*request_merged)(struct request_queue *, struct request *, int);
+ void (*request_merged)(struct request_queue *, struct request *, enum elv_merge);
void (*requests_merged)(struct request_queue *, struct request *, struct request *);
struct request *(*get_request)(struct request_queue *, unsigned int, struct blk_mq_alloc_data *);
void (*put_request)(struct request *);
@@ -166,10 +175,12 @@ extern void elv_dispatch_sort(struct request_queue *, struct request *);
extern void elv_dispatch_add_tail(struct request_queue *, struct request *);
extern void elv_add_request(struct request_queue *, struct request *, int);
extern void __elv_add_request(struct request_queue *, struct request *, int);
-extern int elv_merge(struct request_queue *, struct request **, struct bio *);
+extern enum elv_merge elv_merge(struct request_queue *, struct request **,
+ struct bio *);
extern void elv_merge_requests(struct request_queue *, struct request *,
struct request *);
-extern void elv_merged_request(struct request_queue *, struct request *, int);
+extern void elv_merged_request(struct request_queue *, struct request *,
+ enum elv_merge);
extern void elv_bio_merged(struct request_queue *q, struct request *,
struct bio *);
extern bool elv_attempt_insert_merge(struct request_queue *, struct request *);
@@ -219,13 +230,6 @@ extern void elv_rb_del(struct rb_root *, struct request *);
extern struct request *elv_rb_find(struct rb_root *, sector_t);
/*
- * Return values from elevator merger
- */
-#define ELEVATOR_NO_MERGE 0
-#define ELEVATOR_FRONT_MERGE 1
-#define ELEVATOR_BACK_MERGE 2
-
-/*
* Insertion selection
*/
#define ELEVATOR_INSERT_FRONT 1
--
2.11.0
^ permalink raw reply related
* [PATCH 1/4] block: move req_set_nomerge to blk.h
From: Christoph Hellwig @ 2017-02-08 13:46 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, linux-nvme
In-Reply-To: <20170208134650.5380-1-hch@lst.de>
This makes it available outside of blk-merge.c, and inlining such a trivial
helper seems pretty useful to start with.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/blk-merge.c | 7 -------
block/blk.h | 7 +++++++
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/block/blk-merge.c b/block/blk-merge.c
index a373416dbc9a..c956d9e7aafd 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -482,13 +482,6 @@ int blk_rq_map_sg(struct request_queue *q, struct request *rq,
}
EXPORT_SYMBOL(blk_rq_map_sg);
-static void req_set_nomerge(struct request_queue *q, struct request *req)
-{
- req->cmd_flags |= REQ_NOMERGE;
- if (req == q->last_merge)
- q->last_merge = NULL;
-}
-
static inline int ll_new_hw_segment(struct request_queue *q,
struct request *req,
struct bio *bio)
diff --git a/block/blk.h b/block/blk.h
index 4972b98d47e1..3e08703902a9 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -256,6 +256,13 @@ static inline int blk_do_io_stat(struct request *rq)
!blk_rq_is_passthrough(rq);
}
+static inline void req_set_nomerge(struct request_queue *q, struct request *req)
+{
+ req->cmd_flags |= REQ_NOMERGE;
+ if (req == q->last_merge)
+ q->last_merge = NULL;
+}
+
/*
* Internal io_context interface
*/
--
2.11.0
^ permalink raw reply related
* support for multi-range discard requests V3
From: Christoph Hellwig @ 2017-02-08 13:46 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, linux-nvme
Hi all,
this series adds support for merging discontiguous discard bios into a
single request if the driver supports it. This reduces the number of
discards sent to the device by about a factor of 5-6 for typical
workloads on NVMe, and for slower devices that use I/O scheduling
the number will probably be even bigger but I've not implemented
that support yet.
Changes since V2:
- really fix nr of NVMe ranges to 256
- free range on error
Changes since V1:
- hardcoded max ranges for NVMe to 255
- minor cleanups
^ permalink raw reply
* Re: [PATCH 4/4] nvme: support ranged discard requests
From: Christoph Hellwig @ 2017-02-08 13:36 UTC (permalink / raw)
To: Andrey Kuzmin; +Cc: Christoph Hellwig, Jens Axboe, linux-block, linux-nvme
In-Reply-To: <CANvN+ekrmaCZaYt02wX1DRz4+hTLicjyP-tYDTTcKsWe2Lo5MQ@mail.gmail.com>
On Wed, Feb 08, 2017 at 01:32:22PM +0000, Andrey Kuzmin wrote:
> On Wed, Feb 8, 2017, 16:14 Christoph Hellwig <hch@lst.de> wrote:
>
> > NVMe supports up to 255 ranges per DSM command,
>
>
> 256 per spec, with number of ranges being zero-based.
You're right. I'll do another round of the patch. That beeing
said in my tests I never even managed to get three-digit ranges
anyway with any workload.
^ permalink raw reply
* Re: [PATCH 4/4] nvme: support ranged discard requests
From: Andrey Kuzmin @ 2017-02-08 13:32 UTC (permalink / raw)
To: Christoph Hellwig, Jens Axboe; +Cc: linux-block, linux-nvme
In-Reply-To: <20170208131343.4003-5-hch@lst.de>
[-- Attachment #1: Type: text/plain, Size: 3940 bytes --]
On Wed, Feb 8, 2017, 16:14 Christoph Hellwig <hch@lst.de> wrote:
> NVMe supports up to 255 ranges per DSM command,
256 per spec, with number of ranges being zero-based.
Regards,
Andrey
so wire up support
> for ranged discards up to that limit.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/nvme/host/core.c | 28 +++++++++++++++++++++-------
> include/linux/nvme.h | 2 ++
> 2 files changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index efe8ec300126..3fb25d1d0512 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -238,26 +238,36 @@ static inline void nvme_setup_flush(struct nvme_ns
> *ns,
> static inline int nvme_setup_discard(struct nvme_ns *ns, struct request
> *req,
> struct nvme_command *cmnd)
> {
> + unsigned short segments = blk_rq_nr_discard_segments(req), n = 0;
> struct nvme_dsm_range *range;
> - unsigned int nr_bytes = blk_rq_bytes(req);
> + struct bio *bio;
>
> - range = kmalloc(sizeof(*range), GFP_ATOMIC);
> + range = kmalloc_array(segments, sizeof(*range), GFP_ATOMIC);
> if (!range)
> return BLK_MQ_RQ_QUEUE_BUSY;
>
> - range->cattr = cpu_to_le32(0);
> - range->nlb = cpu_to_le32(nr_bytes >> ns->lba_shift);
> - range->slba = cpu_to_le64(nvme_block_nr(ns, blk_rq_pos(req)));
> + __rq_for_each_bio(bio, req) {
> + u64 slba = nvme_block_nr(ns, bio->bi_iter.bi_sector);
> + u32 nlb = bio->bi_iter.bi_size >> ns->lba_shift;
> +
> + range[n].cattr = cpu_to_le32(0);
> + range[n].nlb = cpu_to_le32(nlb);
> + range[n].slba = cpu_to_le64(slba);
> + n++;
> + }
> +
> + if (WARN_ON_ONCE(n != segments))
> + return BLK_MQ_RQ_QUEUE_ERROR;
>
> memset(cmnd, 0, sizeof(*cmnd));
> cmnd->dsm.opcode = nvme_cmd_dsm;
> cmnd->dsm.nsid = cpu_to_le32(ns->ns_id);
> - cmnd->dsm.nr = 0;
> + cmnd->dsm.nr = segments - 1;
> cmnd->dsm.attributes = cpu_to_le32(NVME_DSMGMT_AD);
>
> req->special_vec.bv_page = virt_to_page(range);
> req->special_vec.bv_offset = offset_in_page(range);
> - req->special_vec.bv_len = sizeof(*range);
> + req->special_vec.bv_len = sizeof(*range) * segments;
> req->rq_flags |= RQF_SPECIAL_PAYLOAD;
>
> return BLK_MQ_RQ_QUEUE_OK;
> @@ -877,6 +887,9 @@ static void nvme_config_discard(struct nvme_ns *ns)
> struct nvme_ctrl *ctrl = ns->ctrl;
> u32 logical_block_size = queue_logical_block_size(ns->queue);
>
> + BUILD_BUG_ON(PAGE_SIZE / sizeof(struct nvme_dsm_range) <
> + NVME_DSM_MAX_RANGES);
> +
> if (ctrl->quirks & NVME_QUIRK_DISCARD_ZEROES)
> ns->queue->limits.discard_zeroes_data = 1;
> else
> @@ -885,6 +898,7 @@ static void nvme_config_discard(struct nvme_ns *ns)
> ns->queue->limits.discard_alignment = logical_block_size;
> ns->queue->limits.discard_granularity = logical_block_size;
> blk_queue_max_discard_sectors(ns->queue, UINT_MAX);
> + blk_queue_max_discard_segments(ns->queue, NVME_DSM_MAX_RANGES);
> queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, ns->queue);
> }
>
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index 3d1c6f1b15c9..dcea305157c2 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -553,6 +553,8 @@ enum {
> NVME_DSMGMT_AD = 1 << 2,
> };
>
> +#define NVME_DSM_MAX_RANGES 255
> +
> struct nvme_dsm_range {
> __le32 cattr;
> __le32 nlb;
> --
> 2.11.0
>
>
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme
>
--
Regards,
Andrey
[-- Attachment #2: Type: text/html, Size: 7318 bytes --]
^ permalink raw reply
* [PATCH 4/4] nvme: support ranged discard requests
From: Christoph Hellwig @ 2017-02-08 13:13 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, linux-nvme
In-Reply-To: <20170208131343.4003-1-hch@lst.de>
NVMe supports up to 255 ranges per DSM command, so wire up support
for ranged discards up to that limit.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/nvme/host/core.c | 28 +++++++++++++++++++++-------
include/linux/nvme.h | 2 ++
2 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index efe8ec300126..3fb25d1d0512 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -238,26 +238,36 @@ static inline void nvme_setup_flush(struct nvme_ns *ns,
static inline int nvme_setup_discard(struct nvme_ns *ns, struct request *req,
struct nvme_command *cmnd)
{
+ unsigned short segments = blk_rq_nr_discard_segments(req), n = 0;
struct nvme_dsm_range *range;
- unsigned int nr_bytes = blk_rq_bytes(req);
+ struct bio *bio;
- range = kmalloc(sizeof(*range), GFP_ATOMIC);
+ range = kmalloc_array(segments, sizeof(*range), GFP_ATOMIC);
if (!range)
return BLK_MQ_RQ_QUEUE_BUSY;
- range->cattr = cpu_to_le32(0);
- range->nlb = cpu_to_le32(nr_bytes >> ns->lba_shift);
- range->slba = cpu_to_le64(nvme_block_nr(ns, blk_rq_pos(req)));
+ __rq_for_each_bio(bio, req) {
+ u64 slba = nvme_block_nr(ns, bio->bi_iter.bi_sector);
+ u32 nlb = bio->bi_iter.bi_size >> ns->lba_shift;
+
+ range[n].cattr = cpu_to_le32(0);
+ range[n].nlb = cpu_to_le32(nlb);
+ range[n].slba = cpu_to_le64(slba);
+ n++;
+ }
+
+ if (WARN_ON_ONCE(n != segments))
+ return BLK_MQ_RQ_QUEUE_ERROR;
memset(cmnd, 0, sizeof(*cmnd));
cmnd->dsm.opcode = nvme_cmd_dsm;
cmnd->dsm.nsid = cpu_to_le32(ns->ns_id);
- cmnd->dsm.nr = 0;
+ cmnd->dsm.nr = segments - 1;
cmnd->dsm.attributes = cpu_to_le32(NVME_DSMGMT_AD);
req->special_vec.bv_page = virt_to_page(range);
req->special_vec.bv_offset = offset_in_page(range);
- req->special_vec.bv_len = sizeof(*range);
+ req->special_vec.bv_len = sizeof(*range) * segments;
req->rq_flags |= RQF_SPECIAL_PAYLOAD;
return BLK_MQ_RQ_QUEUE_OK;
@@ -877,6 +887,9 @@ static void nvme_config_discard(struct nvme_ns *ns)
struct nvme_ctrl *ctrl = ns->ctrl;
u32 logical_block_size = queue_logical_block_size(ns->queue);
+ BUILD_BUG_ON(PAGE_SIZE / sizeof(struct nvme_dsm_range) <
+ NVME_DSM_MAX_RANGES);
+
if (ctrl->quirks & NVME_QUIRK_DISCARD_ZEROES)
ns->queue->limits.discard_zeroes_data = 1;
else
@@ -885,6 +898,7 @@ static void nvme_config_discard(struct nvme_ns *ns)
ns->queue->limits.discard_alignment = logical_block_size;
ns->queue->limits.discard_granularity = logical_block_size;
blk_queue_max_discard_sectors(ns->queue, UINT_MAX);
+ blk_queue_max_discard_segments(ns->queue, NVME_DSM_MAX_RANGES);
queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, ns->queue);
}
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 3d1c6f1b15c9..dcea305157c2 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -553,6 +553,8 @@ enum {
NVME_DSMGMT_AD = 1 << 2,
};
+#define NVME_DSM_MAX_RANGES 255
+
struct nvme_dsm_range {
__le32 cattr;
__le32 nlb;
--
2.11.0
^ permalink raw reply related
* [PATCH 3/4] block: optionally merge discontiguous discard bios into a single request
From: Christoph Hellwig @ 2017-02-08 13:13 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, linux-nvme
In-Reply-To: <20170208131343.4003-1-hch@lst.de>
Add a new merge strategy that merges discard bios into a request until the
maximum number of discard ranges (or the maximum discard size) is reached
from the plug merging code. I/O scheduler merging is not wired up yet
but might also be useful, although not for fast devices like NVMe which
are the only user for now.
Note that for now we don't support limiting the size of each discard range,
but if needed that can be added later.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/blk-core.c | 27 +++++++++++++++++++++++++++
block/blk-merge.c | 5 ++++-
block/blk-mq.c | 3 +++
block/blk-settings.c | 20 ++++++++++++++++++++
block/blk-sysfs.c | 12 ++++++++++++
block/blk.h | 2 ++
include/linux/blkdev.h | 26 ++++++++++++++++++++++++++
include/linux/elevator.h | 1 +
8 files changed, 95 insertions(+), 1 deletion(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index 75fe534861df..b9e857f4afe8 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1485,6 +1485,30 @@ bool bio_attempt_front_merge(struct request_queue *q, struct request *req,
return true;
}
+bool bio_attempt_discard_merge(struct request_queue *q, struct request *req,
+ struct bio *bio)
+{
+ unsigned short segments = blk_rq_nr_discard_segments(req);
+
+ if (segments >= queue_max_discard_segments(q))
+ goto no_merge;
+ if (blk_rq_sectors(req) + bio_sectors(bio) >
+ blk_rq_get_max_sectors(req, blk_rq_pos(req)))
+ goto no_merge;
+
+ req->biotail->bi_next = bio;
+ req->biotail = bio;
+ req->__data_len += bio->bi_iter.bi_size;
+ req->ioprio = ioprio_best(req->ioprio, bio_prio(bio));
+ req->nr_phys_segments = segments + 1;
+
+ blk_account_io_start(req, false);
+ return true;
+no_merge:
+ req_set_nomerge(q, req);
+ return false;
+}
+
/**
* blk_attempt_plug_merge - try to merge with %current's plugged list
* @q: request_queue new bio is being queued at
@@ -1549,6 +1573,9 @@ bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio,
case ELEVATOR_FRONT_MERGE:
merged = bio_attempt_front_merge(q, rq, bio);
break;
+ case ELEVATOR_DISCARD_MERGE:
+ merged = bio_attempt_discard_merge(q, rq, bio);
+ break;
default:
break;
}
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 6cbd90ad5f90..2afa262425d1 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -803,7 +803,10 @@ bool blk_rq_merge_ok(struct request *rq, struct bio *bio)
enum elv_merge blk_try_merge(struct request *rq, struct bio *bio)
{
- if (blk_rq_pos(rq) + blk_rq_sectors(rq) == bio->bi_iter.bi_sector)
+ if (req_op(rq) == REQ_OP_DISCARD &&
+ queue_max_discard_segments(rq->q) > 1)
+ return ELEVATOR_DISCARD_MERGE;
+ else if (blk_rq_pos(rq) + blk_rq_sectors(rq) == bio->bi_iter.bi_sector)
return ELEVATOR_BACK_MERGE;
else if (blk_rq_pos(rq) - bio_sectors(bio) == bio->bi_iter.bi_sector)
return ELEVATOR_FRONT_MERGE;
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 9294633759d9..89cb2d224488 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -780,6 +780,9 @@ static bool blk_mq_attempt_merge(struct request_queue *q,
if (blk_mq_sched_allow_merge(q, rq, bio))
merged = bio_attempt_front_merge(q, rq, bio);
break;
+ case ELEVATOR_DISCARD_MERGE:
+ merged = bio_attempt_discard_merge(q, rq, bio);
+ break;
default:
continue;
}
diff --git a/block/blk-settings.c b/block/blk-settings.c
index 6eb19bcbf3cb..1e7174ffc9d4 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -88,6 +88,7 @@ EXPORT_SYMBOL_GPL(blk_queue_lld_busy);
void blk_set_default_limits(struct queue_limits *lim)
{
lim->max_segments = BLK_MAX_SEGMENTS;
+ lim->max_discard_segments = 1;
lim->max_integrity_segments = 0;
lim->seg_boundary_mask = BLK_SEG_BOUNDARY_MASK;
lim->virt_boundary_mask = 0;
@@ -128,6 +129,7 @@ void blk_set_stacking_limits(struct queue_limits *lim)
/* Inherit limits from component devices */
lim->discard_zeroes_data = 1;
lim->max_segments = USHRT_MAX;
+ lim->max_discard_segments = 1;
lim->max_hw_sectors = UINT_MAX;
lim->max_segment_size = UINT_MAX;
lim->max_sectors = UINT_MAX;
@@ -337,6 +339,22 @@ void blk_queue_max_segments(struct request_queue *q, unsigned short max_segments
EXPORT_SYMBOL(blk_queue_max_segments);
/**
+ * blk_queue_max_discard_segments - set max segments for discard requests
+ * @q: the request queue for the device
+ * @max_segments: max number of segments
+ *
+ * Description:
+ * Enables a low level driver to set an upper limit on the number of
+ * segments in a discard request.
+ **/
+void blk_queue_max_discard_segments(struct request_queue *q,
+ unsigned short max_segments)
+{
+ q->limits.max_discard_segments = max_segments;
+}
+EXPORT_SYMBOL_GPL(blk_queue_max_discard_segments);
+
+/**
* blk_queue_max_segment_size - set max segment size for blk_rq_map_sg
* @q: the request queue for the device
* @max_size: max size of segment in bytes
@@ -553,6 +571,8 @@ int blk_stack_limits(struct queue_limits *t, struct queue_limits *b,
b->virt_boundary_mask);
t->max_segments = min_not_zero(t->max_segments, b->max_segments);
+ t->max_discard_segments = min_not_zero(t->max_discard_segments,
+ b->max_discard_segments);
t->max_integrity_segments = min_not_zero(t->max_integrity_segments,
b->max_integrity_segments);
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 48032c4759a7..070d81bae1d5 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -121,6 +121,12 @@ static ssize_t queue_max_segments_show(struct request_queue *q, char *page)
return queue_var_show(queue_max_segments(q), (page));
}
+static ssize_t queue_max_discard_segments_show(struct request_queue *q,
+ char *page)
+{
+ return queue_var_show(queue_max_discard_segments(q), (page));
+}
+
static ssize_t queue_max_integrity_segments_show(struct request_queue *q, char *page)
{
return queue_var_show(q->limits.max_integrity_segments, (page));
@@ -545,6 +551,11 @@ static struct queue_sysfs_entry queue_max_segments_entry = {
.show = queue_max_segments_show,
};
+static struct queue_sysfs_entry queue_max_discard_segments_entry = {
+ .attr = {.name = "max_discard_segments", .mode = S_IRUGO },
+ .show = queue_max_discard_segments_show,
+};
+
static struct queue_sysfs_entry queue_max_integrity_segments_entry = {
.attr = {.name = "max_integrity_segments", .mode = S_IRUGO },
.show = queue_max_integrity_segments_show,
@@ -697,6 +708,7 @@ static struct attribute *default_attrs[] = {
&queue_max_hw_sectors_entry.attr,
&queue_max_sectors_entry.attr,
&queue_max_segments_entry.attr,
+ &queue_max_discard_segments_entry.attr,
&queue_max_integrity_segments_entry.attr,
&queue_max_segment_size_entry.attr,
&queue_iosched_entry.attr,
diff --git a/block/blk.h b/block/blk.h
index ae82f2ac4019..d1ea4bd9b9a3 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -100,6 +100,8 @@ bool bio_attempt_front_merge(struct request_queue *q, struct request *req,
struct bio *bio);
bool bio_attempt_back_merge(struct request_queue *q, struct request *req,
struct bio *bio);
+bool bio_attempt_discard_merge(struct request_queue *q, struct request *req,
+ struct bio *bio);
bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio,
unsigned int *request_count,
struct request **same_queue_rq);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index e0bac14347e6..aecca0e7d9ca 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -331,6 +331,7 @@ struct queue_limits {
unsigned short logical_block_size;
unsigned short max_segments;
unsigned short max_integrity_segments;
+ unsigned short max_discard_segments;
unsigned char misaligned;
unsigned char discard_misaligned;
@@ -1146,6 +1147,8 @@ extern void blk_queue_bounce_limit(struct request_queue *, u64);
extern void blk_queue_max_hw_sectors(struct request_queue *, unsigned int);
extern void blk_queue_chunk_sectors(struct request_queue *, unsigned int);
extern void blk_queue_max_segments(struct request_queue *, unsigned short);
+extern void blk_queue_max_discard_segments(struct request_queue *,
+ unsigned short);
extern void blk_queue_max_segment_size(struct request_queue *, unsigned int);
extern void blk_queue_max_discard_sectors(struct request_queue *q,
unsigned int max_discard_sectors);
@@ -1189,6 +1192,15 @@ extern void blk_queue_rq_timeout(struct request_queue *, unsigned int);
extern void blk_queue_flush_queueable(struct request_queue *q, bool queueable);
extern void blk_queue_write_cache(struct request_queue *q, bool enabled, bool fua);
+/*
+ * Number of physical segments as sent to the device.
+ *
+ * Normally this is the number of discontiguous data segments sent by the
+ * submitter. But for data-less command like discard we might have no
+ * actual data segments submitted, but the driver might have to add it's
+ * own special payload. In that case we still return 1 here so that this
+ * special payload will be mapped.
+ */
static inline unsigned short blk_rq_nr_phys_segments(struct request *rq)
{
if (rq->rq_flags & RQF_SPECIAL_PAYLOAD)
@@ -1196,6 +1208,15 @@ static inline unsigned short blk_rq_nr_phys_segments(struct request *rq)
return rq->nr_phys_segments;
}
+/*
+ * Number of discard segments (or ranges) the driver needs to fill in.
+ * Each discard bio merged into a request is counted as one segment.
+ */
+static inline unsigned short blk_rq_nr_discard_segments(struct request *rq)
+{
+ return max_t(unsigned short, rq->nr_phys_segments, 1);
+}
+
extern int blk_rq_map_sg(struct request_queue *, struct request *, struct scatterlist *);
extern void blk_dump_rq_flags(struct request *, char *);
extern long nr_blockdev_pages(void);
@@ -1384,6 +1405,11 @@ static inline unsigned short queue_max_segments(struct request_queue *q)
return q->limits.max_segments;
}
+static inline unsigned short queue_max_discard_segments(struct request_queue *q)
+{
+ return q->limits.max_discard_segments;
+}
+
static inline unsigned int queue_max_segment_size(struct request_queue *q)
{
return q->limits.max_segment_size;
diff --git a/include/linux/elevator.h b/include/linux/elevator.h
index b38b4e651ea6..8265b6330cc2 100644
--- a/include/linux/elevator.h
+++ b/include/linux/elevator.h
@@ -16,6 +16,7 @@ enum elv_merge {
ELEVATOR_NO_MERGE = 0,
ELEVATOR_FRONT_MERGE = 1,
ELEVATOR_BACK_MERGE = 2,
+ ELEVATOR_DISCARD_MERGE = 3,
};
typedef enum elv_merge (elevator_merge_fn) (struct request_queue *, struct request **,
--
2.11.0
^ permalink raw reply related
* [PATCH 2/4] block: enumify ELEVATOR_*_MERGE
From: Christoph Hellwig @ 2017-02-08 13:13 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, linux-nvme
In-Reply-To: <20170208131343.4003-1-hch@lst.de>
Switch these constants to an enum, and make let the compiler ensure that
all callers of blk_try_merge and elv_merge handle all potential values.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/blk-core.c | 76 +++++++++++++++++++++++++-----------------------
block/blk-merge.c | 2 +-
block/blk-mq-sched.c | 35 +++++++++++-----------
block/blk-mq.c | 32 +++++++++-----------
block/blk.h | 2 +-
block/cfq-iosched.c | 4 +--
block/deadline-iosched.c | 12 +++-----
block/elevator.c | 10 ++++---
block/mq-deadline.c | 2 +-
include/linux/elevator.h | 28 ++++++++++--------
10 files changed, 102 insertions(+), 101 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index d161d4ab7052..75fe534861df 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1513,12 +1513,11 @@ bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio,
{
struct blk_plug *plug;
struct request *rq;
- bool ret = false;
struct list_head *plug_list;
plug = current->plug;
if (!plug)
- goto out;
+ return false;
*request_count = 0;
if (q->mq_ops)
@@ -1527,7 +1526,7 @@ bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio,
plug_list = &plug->list;
list_for_each_entry_reverse(rq, plug_list, queuelist) {
- int el_ret;
+ bool merged = false;
if (rq->q == q) {
(*request_count)++;
@@ -1543,19 +1542,22 @@ bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio,
if (rq->q != q || !blk_rq_merge_ok(rq, bio))
continue;
- el_ret = blk_try_merge(rq, bio);
- if (el_ret == ELEVATOR_BACK_MERGE) {
- ret = bio_attempt_back_merge(q, rq, bio);
- if (ret)
- break;
- } else if (el_ret == ELEVATOR_FRONT_MERGE) {
- ret = bio_attempt_front_merge(q, rq, bio);
- if (ret)
- break;
+ switch (blk_try_merge(rq, bio)) {
+ case ELEVATOR_BACK_MERGE:
+ merged = bio_attempt_back_merge(q, rq, bio);
+ break;
+ case ELEVATOR_FRONT_MERGE:
+ merged = bio_attempt_front_merge(q, rq, bio);
+ break;
+ default:
+ break;
}
+
+ if (merged)
+ return true;
}
-out:
- return ret;
+
+ return false;
}
unsigned int blk_plug_queued_count(struct request_queue *q)
@@ -1597,7 +1599,7 @@ void init_request_from_bio(struct request *req, struct bio *bio)
static blk_qc_t blk_queue_bio(struct request_queue *q, struct bio *bio)
{
struct blk_plug *plug;
- int el_ret, where = ELEVATOR_INSERT_SORT;
+ int where = ELEVATOR_INSERT_SORT;
struct request *req, *free;
unsigned int request_count = 0;
unsigned int wb_acct;
@@ -1635,27 +1637,29 @@ static blk_qc_t blk_queue_bio(struct request_queue *q, struct bio *bio)
spin_lock_irq(q->queue_lock);
- el_ret = elv_merge(q, &req, bio);
- if (el_ret == ELEVATOR_BACK_MERGE) {
- if (bio_attempt_back_merge(q, req, bio)) {
- elv_bio_merged(q, req, bio);
- free = attempt_back_merge(q, req);
- if (!free)
- elv_merged_request(q, req, el_ret);
- else
- __blk_put_request(q, free);
- goto out_unlock;
- }
- } else if (el_ret == ELEVATOR_FRONT_MERGE) {
- if (bio_attempt_front_merge(q, req, bio)) {
- elv_bio_merged(q, req, bio);
- free = attempt_front_merge(q, req);
- if (!free)
- elv_merged_request(q, req, el_ret);
- else
- __blk_put_request(q, free);
- goto out_unlock;
- }
+ switch (elv_merge(q, &req, bio)) {
+ case ELEVATOR_BACK_MERGE:
+ if (!bio_attempt_back_merge(q, req, bio))
+ break;
+ elv_bio_merged(q, req, bio);
+ free = attempt_back_merge(q, req);
+ if (free)
+ __blk_put_request(q, free);
+ else
+ elv_merged_request(q, req, ELEVATOR_BACK_MERGE);
+ goto out_unlock;
+ case ELEVATOR_FRONT_MERGE:
+ if (!bio_attempt_front_merge(q, req, bio))
+ break;
+ elv_bio_merged(q, req, bio);
+ free = attempt_front_merge(q, req);
+ if (free)
+ __blk_put_request(q, free);
+ else
+ elv_merged_request(q, req, ELEVATOR_FRONT_MERGE);
+ goto out_unlock;
+ default:
+ break;
}
get_rq:
diff --git a/block/blk-merge.c b/block/blk-merge.c
index c956d9e7aafd..6cbd90ad5f90 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -801,7 +801,7 @@ bool blk_rq_merge_ok(struct request *rq, struct bio *bio)
return true;
}
-int blk_try_merge(struct request *rq, struct bio *bio)
+enum elv_merge blk_try_merge(struct request *rq, struct bio *bio)
{
if (blk_rq_pos(rq) + blk_rq_sectors(rq) == bio->bi_iter.bi_sector)
return ELEVATOR_BACK_MERGE;
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index ee455e7cf9d8..72d0d8361175 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -238,30 +238,29 @@ bool blk_mq_sched_try_merge(struct request_queue *q, struct bio *bio,
struct request **merged_request)
{
struct request *rq;
- int ret;
- ret = elv_merge(q, &rq, bio);
- if (ret == ELEVATOR_BACK_MERGE) {
+ switch (elv_merge(q, &rq, bio)) {
+ case ELEVATOR_BACK_MERGE:
if (!blk_mq_sched_allow_merge(q, rq, bio))
return false;
- if (bio_attempt_back_merge(q, rq, bio)) {
- *merged_request = attempt_back_merge(q, rq);
- if (!*merged_request)
- elv_merged_request(q, rq, ret);
- return true;
- }
- } else if (ret == ELEVATOR_FRONT_MERGE) {
+ if (!bio_attempt_back_merge(q, rq, bio))
+ return false;
+ *merged_request = attempt_back_merge(q, rq);
+ if (!*merged_request)
+ elv_merged_request(q, rq, ELEVATOR_BACK_MERGE);
+ return true;
+ case ELEVATOR_FRONT_MERGE:
if (!blk_mq_sched_allow_merge(q, rq, bio))
return false;
- if (bio_attempt_front_merge(q, rq, bio)) {
- *merged_request = attempt_front_merge(q, rq);
- if (!*merged_request)
- elv_merged_request(q, rq, ret);
- return true;
- }
+ if (!bio_attempt_front_merge(q, rq, bio))
+ return false;
+ *merged_request = attempt_front_merge(q, rq);
+ if (!*merged_request)
+ elv_merged_request(q, rq, ELEVATOR_FRONT_MERGE);
+ return true;
+ default:
+ return false;
}
-
- return false;
}
EXPORT_SYMBOL_GPL(blk_mq_sched_try_merge);
diff --git a/block/blk-mq.c b/block/blk-mq.c
index be183e6115a1..9294633759d9 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -763,7 +763,7 @@ static bool blk_mq_attempt_merge(struct request_queue *q,
int checked = 8;
list_for_each_entry_reverse(rq, &ctx->rq_list, queuelist) {
- int el_ret;
+ bool merged = false;
if (!checked--)
break;
@@ -771,26 +771,22 @@ static bool blk_mq_attempt_merge(struct request_queue *q,
if (!blk_rq_merge_ok(rq, bio))
continue;
- el_ret = blk_try_merge(rq, bio);
- if (el_ret == ELEVATOR_NO_MERGE)
- continue;
-
- if (!blk_mq_sched_allow_merge(q, rq, bio))
+ switch (blk_try_merge(rq, bio)) {
+ case ELEVATOR_BACK_MERGE:
+ if (blk_mq_sched_allow_merge(q, rq, bio))
+ merged = bio_attempt_back_merge(q, rq, bio);
break;
-
- if (el_ret == ELEVATOR_BACK_MERGE) {
- if (bio_attempt_back_merge(q, rq, bio)) {
- ctx->rq_merged++;
- return true;
- }
- break;
- } else if (el_ret == ELEVATOR_FRONT_MERGE) {
- if (bio_attempt_front_merge(q, rq, bio)) {
- ctx->rq_merged++;
- return true;
- }
+ case ELEVATOR_FRONT_MERGE:
+ if (blk_mq_sched_allow_merge(q, rq, bio))
+ merged = bio_attempt_front_merge(q, rq, bio);
break;
+ default:
+ continue;
}
+
+ if (merged)
+ ctx->rq_merged++;
+ return merged;
}
return false;
diff --git a/block/blk.h b/block/blk.h
index 3e08703902a9..ae82f2ac4019 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -215,7 +215,7 @@ int blk_attempt_req_merge(struct request_queue *q, struct request *rq,
void blk_recalc_rq_segments(struct request *rq);
void blk_rq_set_mixed_merge(struct request *rq);
bool blk_rq_merge_ok(struct request *rq, struct bio *bio);
-int blk_try_merge(struct request *rq, struct bio *bio);
+enum elv_merge blk_try_merge(struct request *rq, struct bio *bio);
void blk_queue_congestion_threshold(struct request_queue *q);
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index f0f29ee731e1..921262770636 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -2528,7 +2528,7 @@ static void cfq_remove_request(struct request *rq)
}
}
-static int cfq_merge(struct request_queue *q, struct request **req,
+static enum elv_merge cfq_merge(struct request_queue *q, struct request **req,
struct bio *bio)
{
struct cfq_data *cfqd = q->elevator->elevator_data;
@@ -2544,7 +2544,7 @@ static int cfq_merge(struct request_queue *q, struct request **req,
}
static void cfq_merged_request(struct request_queue *q, struct request *req,
- int type)
+ enum elv_merge type)
{
if (type == ELEVATOR_FRONT_MERGE) {
struct cfq_queue *cfqq = RQ_CFQQ(req);
diff --git a/block/deadline-iosched.c b/block/deadline-iosched.c
index 05fc0ea25a98..c68f6bbc0dcd 100644
--- a/block/deadline-iosched.c
+++ b/block/deadline-iosched.c
@@ -120,12 +120,11 @@ static void deadline_remove_request(struct request_queue *q, struct request *rq)
deadline_del_rq_rb(dd, rq);
}
-static int
+static enum elv_merge
deadline_merge(struct request_queue *q, struct request **req, struct bio *bio)
{
struct deadline_data *dd = q->elevator->elevator_data;
struct request *__rq;
- int ret;
/*
* check for front merge
@@ -138,20 +137,17 @@ deadline_merge(struct request_queue *q, struct request **req, struct bio *bio)
BUG_ON(sector != blk_rq_pos(__rq));
if (elv_bio_merge_ok(__rq, bio)) {
- ret = ELEVATOR_FRONT_MERGE;
- goto out;
+ *req = __rq;
+ return ELEVATOR_FRONT_MERGE;
}
}
}
return ELEVATOR_NO_MERGE;
-out:
- *req = __rq;
- return ret;
}
static void deadline_merged_request(struct request_queue *q,
- struct request *req, int type)
+ struct request *req, enum elv_merge type)
{
struct deadline_data *dd = q->elevator->elevator_data;
diff --git a/block/elevator.c b/block/elevator.c
index 7e4f5880dd64..27ff1ed5a6fa 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -427,11 +427,11 @@ void elv_dispatch_add_tail(struct request_queue *q, struct request *rq)
}
EXPORT_SYMBOL(elv_dispatch_add_tail);
-int elv_merge(struct request_queue *q, struct request **req, struct bio *bio)
+enum elv_merge elv_merge(struct request_queue *q, struct request **req,
+ struct bio *bio)
{
struct elevator_queue *e = q->elevator;
struct request *__rq;
- int ret;
/*
* Levels of merges:
@@ -446,7 +446,8 @@ int elv_merge(struct request_queue *q, struct request **req, struct bio *bio)
* First try one-hit cache.
*/
if (q->last_merge && elv_bio_merge_ok(q->last_merge, bio)) {
- ret = blk_try_merge(q->last_merge, bio);
+ enum elv_merge ret = blk_try_merge(q->last_merge, bio);
+
if (ret != ELEVATOR_NO_MERGE) {
*req = q->last_merge;
return ret;
@@ -514,7 +515,8 @@ bool elv_attempt_insert_merge(struct request_queue *q, struct request *rq)
return ret;
}
-void elv_merged_request(struct request_queue *q, struct request *rq, int type)
+void elv_merged_request(struct request_queue *q, struct request *rq,
+ enum elv_merge type)
{
struct elevator_queue *e = q->elevator;
diff --git a/block/mq-deadline.c b/block/mq-deadline.c
index d68d9c273a66..236121633ca0 100644
--- a/block/mq-deadline.c
+++ b/block/mq-deadline.c
@@ -121,7 +121,7 @@ static void deadline_remove_request(struct request_queue *q, struct request *rq)
}
static void dd_request_merged(struct request_queue *q, struct request *req,
- int type)
+ enum elv_merge type)
{
struct deadline_data *dd = q->elevator->elevator_data;
diff --git a/include/linux/elevator.h b/include/linux/elevator.h
index b5825c4f06f7..b38b4e651ea6 100644
--- a/include/linux/elevator.h
+++ b/include/linux/elevator.h
@@ -9,12 +9,21 @@
struct io_cq;
struct elevator_type;
-typedef int (elevator_merge_fn) (struct request_queue *, struct request **,
+/*
+ * Return values from elevator merger
+ */
+enum elv_merge {
+ ELEVATOR_NO_MERGE = 0,
+ ELEVATOR_FRONT_MERGE = 1,
+ ELEVATOR_BACK_MERGE = 2,
+};
+
+typedef enum elv_merge (elevator_merge_fn) (struct request_queue *, struct request **,
struct bio *);
typedef void (elevator_merge_req_fn) (struct request_queue *, struct request *, struct request *);
-typedef void (elevator_merged_fn) (struct request_queue *, struct request *, int);
+typedef void (elevator_merged_fn) (struct request_queue *, struct request *, enum elv_merge);
typedef int (elevator_allow_bio_merge_fn) (struct request_queue *,
struct request *, struct bio *);
@@ -87,7 +96,7 @@ struct elevator_mq_ops {
bool (*allow_merge)(struct request_queue *, struct request *, struct bio *);
bool (*bio_merge)(struct blk_mq_hw_ctx *, struct bio *);
int (*request_merge)(struct request_queue *q, struct request **, struct bio *);
- void (*request_merged)(struct request_queue *, struct request *, int);
+ void (*request_merged)(struct request_queue *, struct request *, enum elv_merge);
void (*requests_merged)(struct request_queue *, struct request *, struct request *);
struct request *(*get_request)(struct request_queue *, unsigned int, struct blk_mq_alloc_data *);
void (*put_request)(struct request *);
@@ -166,10 +175,12 @@ extern void elv_dispatch_sort(struct request_queue *, struct request *);
extern void elv_dispatch_add_tail(struct request_queue *, struct request *);
extern void elv_add_request(struct request_queue *, struct request *, int);
extern void __elv_add_request(struct request_queue *, struct request *, int);
-extern int elv_merge(struct request_queue *, struct request **, struct bio *);
+extern enum elv_merge elv_merge(struct request_queue *, struct request **,
+ struct bio *);
extern void elv_merge_requests(struct request_queue *, struct request *,
struct request *);
-extern void elv_merged_request(struct request_queue *, struct request *, int);
+extern void elv_merged_request(struct request_queue *, struct request *,
+ enum elv_merge);
extern void elv_bio_merged(struct request_queue *q, struct request *,
struct bio *);
extern bool elv_attempt_insert_merge(struct request_queue *, struct request *);
@@ -219,13 +230,6 @@ extern void elv_rb_del(struct rb_root *, struct request *);
extern struct request *elv_rb_find(struct rb_root *, sector_t);
/*
- * Return values from elevator merger
- */
-#define ELEVATOR_NO_MERGE 0
-#define ELEVATOR_FRONT_MERGE 1
-#define ELEVATOR_BACK_MERGE 2
-
-/*
* Insertion selection
*/
#define ELEVATOR_INSERT_FRONT 1
--
2.11.0
^ permalink raw reply related
* [PATCH 1/4] block: move req_set_nomerge to blk.h
From: Christoph Hellwig @ 2017-02-08 13:13 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, linux-nvme
In-Reply-To: <20170208131343.4003-1-hch@lst.de>
This makes it available outside of blk-merge.c, and inlining such a trivial
helper seems pretty useful to start with.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/blk-merge.c | 7 -------
block/blk.h | 7 +++++++
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/block/blk-merge.c b/block/blk-merge.c
index a373416dbc9a..c956d9e7aafd 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -482,13 +482,6 @@ int blk_rq_map_sg(struct request_queue *q, struct request *rq,
}
EXPORT_SYMBOL(blk_rq_map_sg);
-static void req_set_nomerge(struct request_queue *q, struct request *req)
-{
- req->cmd_flags |= REQ_NOMERGE;
- if (req == q->last_merge)
- q->last_merge = NULL;
-}
-
static inline int ll_new_hw_segment(struct request_queue *q,
struct request *req,
struct bio *bio)
diff --git a/block/blk.h b/block/blk.h
index 4972b98d47e1..3e08703902a9 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -256,6 +256,13 @@ static inline int blk_do_io_stat(struct request *rq)
!blk_rq_is_passthrough(rq);
}
+static inline void req_set_nomerge(struct request_queue *q, struct request *req)
+{
+ req->cmd_flags |= REQ_NOMERGE;
+ if (req == q->last_merge)
+ q->last_merge = NULL;
+}
+
/*
* Internal io_context interface
*/
--
2.11.0
^ permalink raw reply related
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