* Re: [PATCH] opal: Use empty structure when not defined
From: Christoph Hellwig @ 2017-02-16 17:21 UTC (permalink / raw)
To: Jon Derrick
Cc: Christoph Hellwig, Keith Busch, linux-block, Scott Bauer,
Jens Axboe
In-Reply-To: <84b78f2d-0238-0fa1-e0b3-d46e7d4cb9be@intel.com>
On Thu, Feb 16, 2017 at 10:18:59AM -0700, Jon Derrick wrote:
> It looks good to me at first glance but I can't apply it. What tree are
> you on?
It's against Jens' for-next tree (actually from two days ago, but
nothing in this area should have changed since then)
^ permalink raw reply
* Re: [PATCH] opal: Use empty structure when not defined
From: Jon Derrick @ 2017-02-16 17:18 UTC (permalink / raw)
To: Christoph Hellwig, Keith Busch; +Cc: linux-block, Scott Bauer, Jens Axboe
In-Reply-To: <20170216075812.GA7662@infradead.org>
It looks good to me at first glance but I can't apply it. What tree are
you on?
On 02/16/2017 12:58 AM, Christoph Hellwig wrote:
> I'd rather prefer to make the structure separately allocated as
> discussed before. Scott, can you test the patch below? I'm not near
> my devices I could test on.
>
> ---
> From b2cda0c7ec5c0ec66582655751838f519cfa1706 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Thu, 16 Feb 2017 08:49:56 +0100
> Subject: block/sed-opal: make struct opal_dev private
>
> This moves the definition of struct opal_dev into sed-opal.c. For this a
> new private data field is added to it that is passed to the send/receive
> callback. After that a lot of internals can be made private.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> block/opal_proto.h | 23 ++++++++++
> block/sed-opal.c | 101 +++++++++++++++++++++++++++++++++++++----
> drivers/nvme/host/core.c | 9 ++--
> drivers/nvme/host/nvme.h | 14 ++----
> drivers/nvme/host/pci.c | 8 +++-
> include/linux/sed-opal.h | 116 ++---------------------------------------------
> 6 files changed, 131 insertions(+), 140 deletions(-)
>
> diff --git a/block/opal_proto.h b/block/opal_proto.h
> index af9abc56c157..f40c9acf8895 100644
> --- a/block/opal_proto.h
> +++ b/block/opal_proto.h
> @@ -19,6 +19,29 @@
> #ifndef _OPAL_PROTO_H
> #define _OPAL_PROTO_H
>
> +/*
> + * These constant values come from:
> + * SPC-4 section
> + * 6.30 SECURITY PROTOCOL IN command / table 265.
> + */
> +enum {
> + TCG_SECP_00 = 0,
> + TCG_SECP_01,
> +};
> +
> +/*
> + * Token defs derived from:
> + * TCG_Storage_Architecture_Core_Spec_v2.01_r1.00
> + * 3.2.2 Data Stream Encoding
> + */
> +enum opal_response_token {
> + OPAL_DTA_TOKENID_BYTESTRING = 0xe0,
> + OPAL_DTA_TOKENID_SINT = 0xe1,
> + OPAL_DTA_TOKENID_UINT = 0xe2,
> + OPAL_DTA_TOKENID_TOKEN = 0xe3, /* actual token is returned */
> + OPAL_DTA_TOKENID_INVALID = 0X0
> +};
> +
> #define DTAERROR_NO_METHOD_STATUS 0x89
> #define GENERIC_HOST_SESSION_NUM 0x41
>
> diff --git a/block/sed-opal.c b/block/sed-opal.c
> index bf1406e5159b..bdab4dfcbafd 100644
> --- a/block/sed-opal.c
> +++ b/block/sed-opal.c
> @@ -31,6 +31,77 @@
>
> #include "opal_proto.h"
>
> +#define IO_BUFFER_LENGTH 2048
> +#define MAX_TOKS 64
> +
> +typedef int (*opal_step)(struct opal_dev *dev);
> +
> +enum opal_atom_width {
> + OPAL_WIDTH_TINY,
> + OPAL_WIDTH_SHORT,
> + OPAL_WIDTH_MEDIUM,
> + OPAL_WIDTH_LONG,
> + OPAL_WIDTH_TOKEN
> +};
> +
> +/*
> + * On the parsed response, we don't store again the toks that are already
> + * stored in the response buffer. Instead, for each token, we just store a
> + * pointer to the position in the buffer where the token starts, and the size
> + * of the token in bytes.
> + */
> +struct opal_resp_tok {
> + const u8 *pos;
> + size_t len;
> + enum opal_response_token type;
> + enum opal_atom_width width;
> + union {
> + u64 u;
> + s64 s;
> + } stored;
> +};
> +
> +/*
> + * From the response header it's not possible to know how many tokens there are
> + * on the payload. So we hardcode that the maximum will be MAX_TOKS, and later
> + * if we start dealing with messages that have more than that, we can increase
> + * this number. This is done to avoid having to make two passes through the
> + * response, the first one counting how many tokens we have and the second one
> + * actually storing the positions.
> + */
> +struct parsed_resp {
> + int num;
> + struct opal_resp_tok toks[MAX_TOKS];
> +};
> +
> +struct opal_dev {
> + bool supported;
> +
> + void *data;
> + sec_send_recv *send_recv;
> +
> + const opal_step *funcs;
> + void **func_data;
> + int state;
> + struct mutex dev_lock;
> + u16 comid;
> + u32 hsn;
> + u32 tsn;
> + u64 align;
> + u64 lowest_lba;
> +
> + size_t pos;
> + u8 cmd[IO_BUFFER_LENGTH];
> + u8 resp[IO_BUFFER_LENGTH];
> +
> + struct parsed_resp parsed;
> + size_t prev_d_len;
> + void *prev_data;
> +
> + struct list_head unlk_lst;
> +};
> +
> +
> static const u8 opaluid[][OPAL_UID_LENGTH] = {
> /* users */
> [OPAL_SMUID_UID] =
> @@ -243,14 +314,14 @@ static u16 get_comid_v200(const void *data)
>
> static int opal_send_cmd(struct opal_dev *dev)
> {
> - return dev->send_recv(dev, dev->comid, TCG_SECP_01,
> + return dev->send_recv(dev->data, dev->comid, TCG_SECP_01,
> dev->cmd, IO_BUFFER_LENGTH,
> true);
> }
>
> static int opal_recv_cmd(struct opal_dev *dev)
> {
> - return dev->send_recv(dev, dev->comid, TCG_SECP_01,
> + return dev->send_recv(dev->data, dev->comid, TCG_SECP_01,
> dev->resp, IO_BUFFER_LENGTH,
> false);
> }
> @@ -1943,16 +2014,24 @@ static int check_opal_support(struct opal_dev *dev)
> return ret;
> }
>
> -void init_opal_dev(struct opal_dev *opal_dev, sec_send_recv *send_recv)
> +struct opal_dev *init_opal_dev(void *data, sec_send_recv *send_recv)
> {
> - if (opal_dev->initialized)
> - return;
> - INIT_LIST_HEAD(&opal_dev->unlk_lst);
> - mutex_init(&opal_dev->dev_lock);
> - opal_dev->send_recv = send_recv;
> - if (check_opal_support(opal_dev) < 0)
> + struct opal_dev *dev;
> +
> + dev = kmalloc(sizeof(*dev), GFP_KERNEL);
> + if (!dev)
> + return NULL;
> +
> + INIT_LIST_HEAD(&dev->unlk_lst);
> + mutex_init(&dev->dev_lock);
> + dev->data = data;
> + dev->send_recv = send_recv;
> + if (check_opal_support(dev) < 0) {
> pr_warn("Opal is not supported on this device\n");
> - opal_dev->initialized = true;
> + kfree(dev);
> + return NULL;
> + }
> + return dev;
> }
> EXPORT_SYMBOL(init_opal_dev);
>
> @@ -2350,6 +2429,8 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
>
> if (!capable(CAP_SYS_ADMIN))
> return -EACCES;
> + if (!dev)
> + return -ENOTSUPP;
> if (!dev->supported) {
> pr_err("Not supported\n");
> return -ENOTSUPP;
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index d9f49036819c..84eb86cc4a45 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -811,7 +811,7 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
> return nvme_nvm_ioctl(ns, cmd, arg);
> #endif
> if (is_sed_ioctl(cmd))
> - return sed_ioctl(&ns->ctrl->opal_dev, cmd, arg);
> + return sed_ioctl(ns->ctrl->opal_dev, cmd, arg);
> return -ENOTTY;
> }
> }
> @@ -1084,18 +1084,17 @@ static const struct pr_ops nvme_pr_ops = {
> };
>
> #ifdef CONFIG_BLK_SED_OPAL
> -int nvme_sec_submit(struct opal_dev *dev, u16 spsp, u8 secp,
> - void *buffer, size_t len, bool send)
> +int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len,
> + bool send)
> {
> + struct nvme_ctrl *ctrl = data;
> struct nvme_command cmd;
> - struct nvme_ctrl *ctrl = NULL;
>
> memset(&cmd, 0, sizeof(cmd));
> if (send)
> cmd.common.opcode = nvme_admin_security_send;
> else
> cmd.common.opcode = nvme_admin_security_recv;
> - ctrl = container_of(dev, struct nvme_ctrl, opal_dev);
> cmd.common.nsid = 0;
> cmd.common.cdw10[0] = cpu_to_le32(((u32)secp) << 24 | ((u32)spsp) << 8);
> cmd.common.cdw10[1] = cpu_to_le32(len);
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 569cba14cede..5126c4bbee1a 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -126,7 +126,7 @@ struct nvme_ctrl {
> struct list_head node;
> struct ida ns_ida;
>
> - struct opal_dev opal_dev;
> + struct opal_dev *opal_dev;
>
> char name[12];
> char serial[20];
> @@ -270,16 +270,8 @@ int nvme_init_identify(struct nvme_ctrl *ctrl);
> void nvme_queue_scan(struct nvme_ctrl *ctrl);
> void nvme_remove_namespaces(struct nvme_ctrl *ctrl);
>
> -#ifdef CONFIG_BLK_SED_OPAL
> -int nvme_sec_submit(struct opal_dev *dev, u16 spsp, u8 secp,
> - void *buffer, size_t len, bool send);
> -#else
> -static inline int nvme_sec_submit(struct opal_dev *dev, u16 spsp, u8 secp,
> - void *buffer, size_t len, bool send)
> -{
> - return 0;
> -}
> -#endif /* CONFIG_BLK_DEV_SED_OPAL */
> +int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len,
> + bool send);
>
> #define NVME_NR_AERS 1
> void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index e25d632bd9f2..5db8a38a8b43 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1739,6 +1739,7 @@ static void nvme_pci_free_ctrl(struct nvme_ctrl *ctrl)
> if (dev->ctrl.admin_q)
> blk_put_queue(dev->ctrl.admin_q);
> kfree(dev->queues);
> + kfree(dev->ctrl.opal_dev);
> kfree(dev);
> }
>
> @@ -1788,10 +1789,13 @@ static void nvme_reset_work(struct work_struct *work)
> if (result)
> goto out;
>
> - init_opal_dev(&dev->ctrl.opal_dev, &nvme_sec_submit);
> + if (!dev->ctrl.opal_dev) {
> + dev->ctrl.opal_dev =
> + init_opal_dev(&dev->ctrl, &nvme_sec_submit);
> + }
>
> if (was_suspend)
> - opal_unlock_from_suspend(&dev->ctrl.opal_dev);
> + opal_unlock_from_suspend(dev->ctrl.opal_dev);
>
> result = nvme_setup_io_queues(dev);
> if (result)
> diff --git a/include/linux/sed-opal.h b/include/linux/sed-opal.h
> index af1a85eae193..da39002285d0 100644
> --- a/include/linux/sed-opal.h
> +++ b/include/linux/sed-opal.h
> @@ -21,117 +21,14 @@
> #include <uapi/linux/sed-opal.h>
> #include <linux/kernel.h>
>
> -/*
> - * These constant values come from:
> - * SPC-4 section
> - * 6.30 SECURITY PROTOCOL IN command / table 265.
> - */
> -enum {
> - TCG_SECP_00 = 0,
> - TCG_SECP_01,
> -};
> struct opal_dev;
>
> -#define IO_BUFFER_LENGTH 2048
> -#define MAX_TOKS 64
> -
> -typedef int (*opal_step)(struct opal_dev *dev);
> -typedef int (sec_send_recv)(struct opal_dev *ctx, u16 spsp, u8 secp,
> - void *buffer, size_t len, bool send);
> -
> -
> -enum opal_atom_width {
> - OPAL_WIDTH_TINY,
> - OPAL_WIDTH_SHORT,
> - OPAL_WIDTH_MEDIUM,
> - OPAL_WIDTH_LONG,
> - OPAL_WIDTH_TOKEN
> -};
> -
> -/*
> - * Token defs derived from:
> - * TCG_Storage_Architecture_Core_Spec_v2.01_r1.00
> - * 3.2.2 Data Stream Encoding
> - */
> -enum opal_response_token {
> - OPAL_DTA_TOKENID_BYTESTRING = 0xe0,
> - OPAL_DTA_TOKENID_SINT = 0xe1,
> - OPAL_DTA_TOKENID_UINT = 0xe2,
> - OPAL_DTA_TOKENID_TOKEN = 0xe3, /* actual token is returned */
> - OPAL_DTA_TOKENID_INVALID = 0X0
> -};
> -
> -/*
> - * On the parsed response, we don't store again the toks that are already
> - * stored in the response buffer. Instead, for each token, we just store a
> - * pointer to the position in the buffer where the token starts, and the size
> - * of the token in bytes.
> - */
> -struct opal_resp_tok {
> - const u8 *pos;
> - size_t len;
> - enum opal_response_token type;
> - enum opal_atom_width width;
> - union {
> - u64 u;
> - s64 s;
> - } stored;
> -};
> -
> -/*
> - * From the response header it's not possible to know how many tokens there are
> - * on the payload. So we hardcode that the maximum will be MAX_TOKS, and later
> - * if we start dealing with messages that have more than that, we can increase
> - * this number. This is done to avoid having to make two passes through the
> - * response, the first one counting how many tokens we have and the second one
> - * actually storing the positions.
> - */
> -struct parsed_resp {
> - int num;
> - struct opal_resp_tok toks[MAX_TOKS];
> -};
> -
> -/**
> - * struct opal_dev - The structure representing a OPAL enabled SED.
> - * @supported: Whether or not OPAL is supported on this controller.
> - * @send_recv: The combined sec_send/sec_recv function pointer.
> - * @opal_step: A series of opal methods that are necessary to complete a command.
> - * @func_data: An array of parameters for the opal methods above.
> - * @state: Describes the current opal_step we're working on.
> - * @dev_lock: Locks the entire opal_dev structure.
> - * @parsed: Parsed response from controller.
> - * @prev_data: Data returned from a method to the controller.
> - * @unlk_lst: A list of Locking ranges to unlock on this device during a resume.
> - */
> -struct opal_dev {
> - bool initialized;
> - bool supported;
> - sec_send_recv *send_recv;
> -
> - const opal_step *funcs;
> - void **func_data;
> - int state;
> - struct mutex dev_lock;
> - u16 comid;
> - u32 hsn;
> - u32 tsn;
> - u64 align;
> - u64 lowest_lba;
> -
> - size_t pos;
> - u8 cmd[IO_BUFFER_LENGTH];
> - u8 resp[IO_BUFFER_LENGTH];
> -
> - struct parsed_resp parsed;
> - size_t prev_d_len;
> - void *prev_data;
> -
> - struct list_head unlk_lst;
> -};
> +typedef int (sec_send_recv)(void *data, u16 spsp, u8 secp, void *buffer,
> + size_t len, bool send);
>
> #ifdef CONFIG_BLK_SED_OPAL
> bool opal_unlock_from_suspend(struct opal_dev *dev);
> -void init_opal_dev(struct opal_dev *opal_dev, sec_send_recv *send_recv);
> +struct opal_dev * init_opal_dev(void *data, sec_send_recv *send_recv);
> int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr);
>
> static inline bool is_sed_ioctl(unsigned int cmd)
> @@ -168,11 +65,6 @@ static inline bool opal_unlock_from_suspend(struct opal_dev *dev)
> {
> return false;
> }
> -static inline void init_opal_dev(struct opal_dev *opal_dev,
> - sec_send_recv *send_recv)
> -{
> - opal_dev->supported = false;
> - opal_dev->initialized = true;
> -}
> +#define init_opal_dev(data, send_recv) NULL
> #endif /* CONFIG_BLK_SED_OPAL */
> #endif /* LINUX_OPAL_H */
>
^ permalink raw reply
* Re: [PATCHv3 4/4] MAINTAINERS: Remove powerpc's opal match
From: Jon Derrick @ 2017-02-16 16:14 UTC (permalink / raw)
To: Michael Ellerman
Cc: Jens Axboe, Rafael Antognolli, Greg Kroah-Hartman, linux-kernel,
linux-block, linuxppc-dev, Christoph Hellwig, Scott Bauer
In-Reply-To: <87poiik8tb.fsf@concordia.ellerman.id.au>
Thanks everyone. Sorry about the mess :)
On 02/15/2017 10:23 PM, Michael Ellerman wrote:
> Jon Derrick <jonathan.derrick@intel.com> writes:
>
>> PPC's 'opal' match pattern also matches block/sed-opal.c, where it looks
>> like the 'arch/powerpc' file pattern should be enough to match powerpc
>> opal code by itself. Remove the opal regex pattern from powerpc.
>
> We thought of it first.
>
> Can't you just rename your driver, Opal Storage Specification, so "oss",
> that should be pretty unique?
>
> ... :)
>
> I don't like this version, but I'll merge the one from Stewart which
> drops the pattern and adds the paths for the existing drivers.
>
> cheers
>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index b983b25..430dd02 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -7404,7 +7404,6 @@ F: drivers/pci/hotplug/pnv_php.c
>> F: drivers/pci/hotplug/rpa*
>> F: drivers/scsi/ibmvscsi/
>> F: tools/testing/selftests/powerpc
>> -N: opal
>> N: /pmac
>> N: powermac
>> N: powernv
>> --
>> 1.8.3.1
^ permalink raw reply
* Re: [PATCH 7/8] mq-deadline: add blk-mq adaptation of the deadline IO scheduler
From: Jens Axboe @ 2017-02-16 15:35 UTC (permalink / raw)
To: Paolo Valente; +Cc: Jens Axboe, linux-block, Linux-Kernal, osandov
In-Reply-To: <FC3DABDF-FA97-4E45-84C0-D9554669FF15@linaro.org>
On 02/16/2017 03:46 AM, Paolo Valente wrote:
>
>> Il giorno 17 dic 2016, alle ore 01:12, Jens Axboe <axboe@fb.com> ha scritto:
>>
>> This is basically identical to deadline-iosched, except it registers
>> as a MQ capable scheduler. This is still a single queue design.
>>
>> Signed-off-by: Jens Axboe <axboe@fb.com>
> ...
>> +
>> +static void dd_merged_requests(struct request_queue *q, struct request *req,
>> + struct request *next)
>> +{
>> + /*
>> + * if next expires before rq, assign its expire time to rq
>> + * and move into next position (next will be deleted) in fifo
>> + */
>> + if (!list_empty(&req->queuelist) && !list_empty(&next->queuelist)) {
>> + if (time_before((unsigned long)next->fifo_time,
>> + (unsigned long)req->fifo_time)) {
>> + list_move(&req->queuelist, &next->queuelist);
>> + req->fifo_time = next->fifo_time;
>> + }
>> + }
>> +
>
> Jens,
> while trying to imagine the possible causes of Bart's hang with
> bfq-mq, I've bumped into the following doubt: in the above function
> (in my case, in bfq-mq-'s equivalent of the above function), are
> we sure that neither req or next could EVER be in dd->dispatch instead
> of dd->fifo_list? I've tried to verify it, but, although I think it has never
> happened in my tests, I was not able to make sure that no unlucky
> combination may ever happen (considering also the use of
> blk_rq_is_passthrough too, to decide where to put a new request).
>
> I'm making a blunder, right?
If a request goes into dd->dispatch, it's going to be found for merging.
Hence we can never call the above on the request.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH V2] nbd: ref count the socks array
From: Josef Bacik @ 2017-02-16 15:06 UTC (permalink / raw)
To: linux-block, kernel-team, nbd-general, axboe
In-Reply-To: <1486566305-10039-1-git-send-email-jbacik@fb.com>
On Wed, 2017-02-08 at 10:05 -0500, Josef Bacik wrote:
> 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>
Actually turns out I need to do this slightly differently to deal with
the netlink device addition interface, so I'm just going to drop this
altogether and do it the other way rather than reworking it in another
patch. Thanks,
Josef
^ permalink raw reply
* Re: [BUG] Potential deadlock in the block layer
From: Jens Axboe @ 2017-02-16 14:56 UTC (permalink / raw)
To: Thomas Gleixner
Cc: LKML, linux-block, Gabriel C, Steven Rostedt, Peter Zijlstra,
Linus Torvalds
In-Reply-To: <alpine.DEB.2.20.1702161533280.3543@nanos>
On 02/16/2017 07:35 AM, Thomas Gleixner wrote:
> On Mon, 13 Feb 2017, Jens Axboe wrote:
>
>> On 02/13/2017 07:14 AM, Thomas Gleixner wrote:
>>> Gabriel reported the lockdep splat below while investigating something
>>> different.
>>>
>>> Explanation for the splat is in the function comment above
>>> del_timer_sync().
>>>
>>> I can reproduce it as well and it's clearly broken.
>>
>> Agree, the disable logic is broken. The below isn't super pretty, but it
>> should do the trick for 4.10.
>
> Cures the splat. Is that a new 4.10 'feature'? That wbt stuff has been
> merged a few releases ago.
OK good, I'll get this submitted for 4.10. wbt is a new feature in
this release.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCHv4 3/4] block/sed: Check received header lengths
From: Christoph Hellwig @ 2017-02-16 14:40 UTC (permalink / raw)
To: Jon Derrick
Cc: linux-block, linux-kernel, linuxppc-dev, Scott Bauer,
Rafael Antognolli, Jens Axboe, Christoph Hellwig,
Greg Kroah-Hartman
In-Reply-To: <1487195159-12599-4-git-send-email-jonathan.derrick@intel.com>
On Wed, Feb 15, 2017 at 02:45:56PM -0700, Jon Derrick wrote:
> Add a buffer size check against discovery and response header lengths
> before we loop over their buffers.
>
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> Reviewed-by: Scott Bauer <scott.bauer@intel.com>
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply
* Re: [PATCHv4 2/4] block/sed: Add helper to qualify response tokens
From: Christoph Hellwig @ 2017-02-16 14:40 UTC (permalink / raw)
To: Jon Derrick
Cc: linux-block, linux-kernel, linuxppc-dev, Scott Bauer,
Rafael Antognolli, Jens Axboe, Christoph Hellwig,
Greg Kroah-Hartman
In-Reply-To: <1487195159-12599-3-git-send-email-jonathan.derrick@intel.com>
On Wed, Feb 15, 2017 at 02:45:55PM -0700, Jon Derrick wrote:
> Add helper which verifies the response token is valid and matches the
> expected value. Merges token_type and response_get_token.
>
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> Reviewed-by: Scott Bauer <scott.bauer@intel.com>
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply
* Re: [BUG] Potential deadlock in the block layer
From: Thomas Gleixner @ 2017-02-16 14:35 UTC (permalink / raw)
To: Jens Axboe
Cc: LKML, linux-block, Gabriel C, Steven Rostedt, Peter Zijlstra,
Linus Torvalds
In-Reply-To: <4d3f25fc-7480-769c-2f4f-e5271a47f7c3@kernel.dk>
On Mon, 13 Feb 2017, Jens Axboe wrote:
> On 02/13/2017 07:14 AM, Thomas Gleixner wrote:
> > Gabriel reported the lockdep splat below while investigating something
> > different.
> >
> > Explanation for the splat is in the function comment above
> > del_timer_sync().
> >
> > I can reproduce it as well and it's clearly broken.
>
> Agree, the disable logic is broken. The below isn't super pretty, but it
> should do the trick for 4.10.
Cures the splat. Is that a new 4.10 'feature'? That wbt stuff has been
merged a few releases ago.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCHv4 1/4] block/sed: Use ssize_t on atom parsers to return errors
From: Christoph Hellwig @ 2017-02-16 14:33 UTC (permalink / raw)
To: Jon Derrick
Cc: linux-block, linux-kernel, linuxppc-dev, Scott Bauer,
Rafael Antognolli, Jens Axboe, Christoph Hellwig,
Greg Kroah-Hartman
In-Reply-To: <1487195159-12599-2-git-send-email-jonathan.derrick@intel.com>
On Wed, Feb 15, 2017 at 02:45:54PM -0700, Jon Derrick wrote:
> The short atom parser can return an errno from decoding but does not
> currently return the error as a signed value. Convert all of the parsers
> to ssize_t.
>
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> Reviewed-by: Scott Bauer <scott.bauer@intel.com>
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply
* Re: sense handling improvements
From: Christoph Hellwig @ 2017-02-16 14:33 UTC (permalink / raw)
To: Martin K. Petersen; +Cc: Christoph Hellwig, axboe, linux-block, linux-scsi
In-Reply-To: <yq18tp6by2n.fsf@oracle.com>
On Wed, Feb 15, 2017 at 10:42:56PM -0500, Martin K. Petersen wrote:
> Christoph> The first patch prevents any possibily of reusing stale sense
> Christoph> codes in sense headers, and is a bug fix that we should
> Christoph> probably get into the block tree ASAP.
>
> Christoph> The rest cleans up handling of the parsed sense data and
> Christoph> could go in either through the block tree, or a SCSI branch
> Christoph> on top of the block tree.
>
> I can bring them in after Linus' initial block pull.
That would be great.
^ permalink raw reply
* Re: [PATCH 02/17] block: introduce bio_remove_last_page()
From: Johannes Thumshirn @ 2017-02-16 14:10 UTC (permalink / raw)
To: Ming Lei
Cc: Shaohua Li, Jens Axboe, Linux Kernel Mailing List,
open list:SOFTWARE RAID (Multiple Disks) SUPPORT, linux-block,
Christoph Hellwig, NeilBrown
In-Reply-To: <CACVXFVPLXfWKcQGcmX8H6Um+4P0XFpKC0DWC4-JXy94st=QL7Q@mail.gmail.com>
On 02/16/2017 02:59 PM, Ming Lei wrote:
> On Thu, Feb 16, 2017 at 9:40 PM, Johannes Thumshirn <jthumshirn@suse.de> wrote:
>> On 02/16/2017 02:30 PM, Ming Lei wrote:
>>> On Thu, Feb 16, 2017 at 8:08 PM, Johannes Thumshirn <jthumshirn@suse.de> wrote:
>>>> On 02/16/2017 12:45 PM, Ming Lei wrote:
>>>>> MD need this helper to remove the last added page, so introduce
>>>>> it.
>>>>>
>>>>> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
>>>>> ---
>>>>> block/bio.c | 23 +++++++++++++++++++++++
>>>>> include/linux/bio.h | 1 +
>>>>> 2 files changed, 24 insertions(+)
>>>>>
>>>>> diff --git a/block/bio.c b/block/bio.c
>>>>> index 5eec5e08417f..0ce7ffcd7939 100644
>>>>> --- a/block/bio.c
>>>>> +++ b/block/bio.c
>>>>> @@ -837,6 +837,29 @@ int bio_add_pc_page(struct request_queue *q, struct bio *bio, struct page
>>>>> EXPORT_SYMBOL(bio_add_pc_page);
>>>>>
>>>>> /**
>>>>> + * bio_remove_last_page - remove the last added page
>>>>> + * @bio: destination bio
>>>>> + *
>>>>> + * Attempt to remove the last added page from the bio_vec maplist.
>>>>> + */
>>>>> +void bio_remove_last_page(struct bio *bio)
>>>>> +{
>>>>> + /*
>>>>> + * cloned bio must not modify vec list
>>>>> + */
>>>>> + if (WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)))
>>>>> + return;
>>>>> +
>>>>> + if (bio->bi_vcnt > 0) {
>>>>
>>>> In patch 1 you introduce bio_segments_all() with the log message 'So
>>>> that we can replace the direct access to .bi_vcnt.' Here you introduce a
>>>> new direct access to it (plus the duplicated WARN_ON_ONCE()).
>>>>
>>>> Maybe use the helper directly here (I admit I haven't gone through the
>>>> whole series yet, so I can't see if the change is made later).
>>>
>>> Firstly MD does need one helper to remove the last added page, as you
>>> can see there are three such uses in patch3.
>>>
>>> Secondly both the two helpers will be changed once multipage bvec
>>> is supported, that means we have to change MD too after multipage bvec
>>> if just using bio_segments_all() to replace .bi_vcnt for removing
>>> the last added page.
>>
>> I'm not sure if we're talking past each other here, I assumed you'd do
>> something like:
>>
>> void bio_remove_last_page(struct bio *bio)
>> {
>> int vcnt = bio_segments_all(bio);
>>
>> if (bio_flagged(bio, BIO_CLONED))
>> return;
>>
>> if (vcnt > 0) {
>> struct bio_vec *bv = &bio->bi_io_vec[vcnt - 1];
>>
>> bio->bi_iter.bi_size -= bv->bv_len;
>> bio->bi_vcnt--;
>> }
>> }
>
> What we are doing is to remove the external direct access to bvec
> table in drivers or filesystems because they may misuse that, for
> example, drivers often use .bi_vcnt to get the page count in the bio,
> but the actual meaning is just bvec's count.
>
> And the implementation in block layer still need to play the table directly,
> especially for sake of efficiency, cause we understand the details and won't
> misuse that.
Ah OK, so bio_segments_all() is intended for drivers.
Thanks for the clarification,
Johannes
--
Johannes Thumshirn Storage
jthumshirn@suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
^ permalink raw reply
* Re: [PATCH 02/17] block: introduce bio_remove_last_page()
From: Ming Lei @ 2017-02-16 13:59 UTC (permalink / raw)
To: Johannes Thumshirn
Cc: Shaohua Li, Jens Axboe, Linux Kernel Mailing List,
open list:SOFTWARE RAID (Multiple Disks) SUPPORT, linux-block,
Christoph Hellwig, NeilBrown
In-Reply-To: <d6d67f9b-e011-cc57-e484-e21b66ab9d6b@suse.de>
On Thu, Feb 16, 2017 at 9:40 PM, Johannes Thumshirn <jthumshirn@suse.de> wrote:
> On 02/16/2017 02:30 PM, Ming Lei wrote:
>> On Thu, Feb 16, 2017 at 8:08 PM, Johannes Thumshirn <jthumshirn@suse.de> wrote:
>>> On 02/16/2017 12:45 PM, Ming Lei wrote:
>>>> MD need this helper to remove the last added page, so introduce
>>>> it.
>>>>
>>>> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
>>>> ---
>>>> block/bio.c | 23 +++++++++++++++++++++++
>>>> include/linux/bio.h | 1 +
>>>> 2 files changed, 24 insertions(+)
>>>>
>>>> diff --git a/block/bio.c b/block/bio.c
>>>> index 5eec5e08417f..0ce7ffcd7939 100644
>>>> --- a/block/bio.c
>>>> +++ b/block/bio.c
>>>> @@ -837,6 +837,29 @@ int bio_add_pc_page(struct request_queue *q, struct bio *bio, struct page
>>>> EXPORT_SYMBOL(bio_add_pc_page);
>>>>
>>>> /**
>>>> + * bio_remove_last_page - remove the last added page
>>>> + * @bio: destination bio
>>>> + *
>>>> + * Attempt to remove the last added page from the bio_vec maplist.
>>>> + */
>>>> +void bio_remove_last_page(struct bio *bio)
>>>> +{
>>>> + /*
>>>> + * cloned bio must not modify vec list
>>>> + */
>>>> + if (WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)))
>>>> + return;
>>>> +
>>>> + if (bio->bi_vcnt > 0) {
>>>
>>> In patch 1 you introduce bio_segments_all() with the log message 'So
>>> that we can replace the direct access to .bi_vcnt.' Here you introduce a
>>> new direct access to it (plus the duplicated WARN_ON_ONCE()).
>>>
>>> Maybe use the helper directly here (I admit I haven't gone through the
>>> whole series yet, so I can't see if the change is made later).
>>
>> Firstly MD does need one helper to remove the last added page, as you
>> can see there are three such uses in patch3.
>>
>> Secondly both the two helpers will be changed once multipage bvec
>> is supported, that means we have to change MD too after multipage bvec
>> if just using bio_segments_all() to replace .bi_vcnt for removing
>> the last added page.
>
> I'm not sure if we're talking past each other here, I assumed you'd do
> something like:
>
> void bio_remove_last_page(struct bio *bio)
> {
> int vcnt = bio_segments_all(bio);
>
> if (bio_flagged(bio, BIO_CLONED))
> return;
>
> if (vcnt > 0) {
> struct bio_vec *bv = &bio->bi_io_vec[vcnt - 1];
>
> bio->bi_iter.bi_size -= bv->bv_len;
> bio->bi_vcnt--;
> }
> }
What we are doing is to remove the external direct access to bvec
table in drivers or filesystems because they may misuse that, for
example, drivers often use .bi_vcnt to get the page count in the bio,
but the actual meaning is just bvec's count.
And the implementation in block layer still need to play the table directly,
especially for sake of efficiency, cause we understand the details and won't
misuse that.
Thanks,
Ming Lei
^ permalink raw reply
* Re: [PATCH 02/17] block: introduce bio_remove_last_page()
From: Johannes Thumshirn @ 2017-02-16 13:40 UTC (permalink / raw)
To: Ming Lei
Cc: Shaohua Li, Jens Axboe, Linux Kernel Mailing List,
open list:SOFTWARE RAID (Multiple Disks) SUPPORT, linux-block,
Christoph Hellwig, NeilBrown
In-Reply-To: <CACVXFVPPifd4ktKnCb6v+44dwUOyN6cTNiQp5zT82r9XzP_z1A@mail.gmail.com>
On 02/16/2017 02:30 PM, Ming Lei wrote:
> On Thu, Feb 16, 2017 at 8:08 PM, Johannes Thumshirn <jthumshirn@suse.de> wrote:
>> On 02/16/2017 12:45 PM, Ming Lei wrote:
>>> MD need this helper to remove the last added page, so introduce
>>> it.
>>>
>>> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
>>> ---
>>> block/bio.c | 23 +++++++++++++++++++++++
>>> include/linux/bio.h | 1 +
>>> 2 files changed, 24 insertions(+)
>>>
>>> diff --git a/block/bio.c b/block/bio.c
>>> index 5eec5e08417f..0ce7ffcd7939 100644
>>> --- a/block/bio.c
>>> +++ b/block/bio.c
>>> @@ -837,6 +837,29 @@ int bio_add_pc_page(struct request_queue *q, struct bio *bio, struct page
>>> EXPORT_SYMBOL(bio_add_pc_page);
>>>
>>> /**
>>> + * bio_remove_last_page - remove the last added page
>>> + * @bio: destination bio
>>> + *
>>> + * Attempt to remove the last added page from the bio_vec maplist.
>>> + */
>>> +void bio_remove_last_page(struct bio *bio)
>>> +{
>>> + /*
>>> + * cloned bio must not modify vec list
>>> + */
>>> + if (WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)))
>>> + return;
>>> +
>>> + if (bio->bi_vcnt > 0) {
>>
>> In patch 1 you introduce bio_segments_all() with the log message 'So
>> that we can replace the direct access to .bi_vcnt.' Here you introduce a
>> new direct access to it (plus the duplicated WARN_ON_ONCE()).
>>
>> Maybe use the helper directly here (I admit I haven't gone through the
>> whole series yet, so I can't see if the change is made later).
>
> Firstly MD does need one helper to remove the last added page, as you
> can see there are three such uses in patch3.
>
> Secondly both the two helpers will be changed once multipage bvec
> is supported, that means we have to change MD too after multipage bvec
> if just using bio_segments_all() to replace .bi_vcnt for removing
> the last added page.
I'm not sure if we're talking past each other here, I assumed you'd do
something like:
void bio_remove_last_page(struct bio *bio)
{
int vcnt = bio_segments_all(bio);
if (bio_flagged(bio, BIO_CLONED))
return;
if (vcnt > 0) {
struct bio_vec *bv = &bio->bi_io_vec[vcnt - 1];
bio->bi_iter.bi_size -= bv->bv_len;
bio->bi_vcnt--;
}
}
--
Johannes Thumshirn Storage
jthumshirn@suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
^ permalink raw reply
* Re: [PATCH 13/17] md: raid1: use bio_segments_all()
From: Ming Lei @ 2017-02-16 13:38 UTC (permalink / raw)
To: Johannes Thumshirn
Cc: Shaohua Li, Jens Axboe, Linux Kernel Mailing List,
open list:SOFTWARE RAID (Multiple Disks) SUPPORT, linux-block,
Christoph Hellwig, NeilBrown
In-Reply-To: <fc4d24ec-55a1-7280-f734-c9b8c4a0ecae@suse.de>
On Thu, Feb 16, 2017 at 9:34 PM, Johannes Thumshirn <jthumshirn@suse.de> wrote:
> On 02/16/2017 02:32 PM, Ming Lei wrote:
>> On Thu, Feb 16, 2017 at 8:35 PM, Johannes Thumshirn <jthumshirn@suse.de> wrote:
>>> On 02/16/2017 12:45 PM, Ming Lei wrote:
>>>> @@ -998,7 +998,8 @@ static void alloc_behind_pages(struct bio *bio, struct r1bio *r1_bio)
>>>> {
>>>> int i;
>>>> struct bio_vec *bvec;
>>>> - struct bio_vec *bvecs = kzalloc(bio->bi_vcnt * sizeof(struct bio_vec),
>>>> + unsigned vcnt = bio_segments_all(bio);
>>>> + struct bio_vec *bvecs = kzalloc(vcnt * sizeof(struct bio_vec),
>>>> GFP_NOIO);
>>>
>>> Maybe use kcalloc() instead of kzalloc() with a multiplication.
>>
>> That doesn't belong to this patch, which just wants to remove direct
>> access to .bi_vcnt.
>
> But you're touching it anyways, aren't you?
Don't you know the policy of 'do one thing, do it better' in one patch?
If you want to switch to kcalloc(), just post a patch, that is fine, but
please don't push me to do that in this patch.
Thanks,
Ming Lei
^ permalink raw reply
* Re: [PATCH 13/17] md: raid1: use bio_segments_all()
From: Johannes Thumshirn @ 2017-02-16 13:34 UTC (permalink / raw)
To: Ming Lei
Cc: Shaohua Li, Jens Axboe, Linux Kernel Mailing List,
open list:SOFTWARE RAID (Multiple Disks) SUPPORT, linux-block,
Christoph Hellwig, NeilBrown
In-Reply-To: <CACVXFVOf=4yjNRfemcYEYCc7eeQeL9Esp_WM9ELgX77S1LT+1g@mail.gmail.com>
On 02/16/2017 02:32 PM, Ming Lei wrote:
> On Thu, Feb 16, 2017 at 8:35 PM, Johannes Thumshirn <jthumshirn@suse.de> wrote:
>> On 02/16/2017 12:45 PM, Ming Lei wrote:
>>> @@ -998,7 +998,8 @@ static void alloc_behind_pages(struct bio *bio, struct r1bio *r1_bio)
>>> {
>>> int i;
>>> struct bio_vec *bvec;
>>> - struct bio_vec *bvecs = kzalloc(bio->bi_vcnt * sizeof(struct bio_vec),
>>> + unsigned vcnt = bio_segments_all(bio);
>>> + struct bio_vec *bvecs = kzalloc(vcnt * sizeof(struct bio_vec),
>>> GFP_NOIO);
>>
>> Maybe use kcalloc() instead of kzalloc() with a multiplication.
>
> That doesn't belong to this patch, which just wants to remove direct
> access to .bi_vcnt.
But you're touching it anyways, aren't you?
--
Johannes Thumshirn Storage
jthumshirn@suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
^ permalink raw reply
* Re: [PATCH 13/17] md: raid1: use bio_segments_all()
From: Ming Lei @ 2017-02-16 13:32 UTC (permalink / raw)
To: Johannes Thumshirn
Cc: Shaohua Li, Jens Axboe, Linux Kernel Mailing List,
open list:SOFTWARE RAID (Multiple Disks) SUPPORT, linux-block,
Christoph Hellwig, NeilBrown
In-Reply-To: <4a700de4-8540-d5d8-8c58-3465c0a7a55a@suse.de>
On Thu, Feb 16, 2017 at 8:35 PM, Johannes Thumshirn <jthumshirn@suse.de> wrote:
> On 02/16/2017 12:45 PM, Ming Lei wrote:
>> @@ -998,7 +998,8 @@ static void alloc_behind_pages(struct bio *bio, struct r1bio *r1_bio)
>> {
>> int i;
>> struct bio_vec *bvec;
>> - struct bio_vec *bvecs = kzalloc(bio->bi_vcnt * sizeof(struct bio_vec),
>> + unsigned vcnt = bio_segments_all(bio);
>> + struct bio_vec *bvecs = kzalloc(vcnt * sizeof(struct bio_vec),
>> GFP_NOIO);
>
> Maybe use kcalloc() instead of kzalloc() with a multiplication.
That doesn't belong to this patch, which just wants to remove direct
access to .bi_vcnt.
Thanks,
Ming Lei
^ permalink raw reply
* Re: [PATCH 02/17] block: introduce bio_remove_last_page()
From: Ming Lei @ 2017-02-16 13:30 UTC (permalink / raw)
To: Johannes Thumshirn
Cc: Shaohua Li, Jens Axboe, Linux Kernel Mailing List,
open list:SOFTWARE RAID (Multiple Disks) SUPPORT, linux-block,
Christoph Hellwig, NeilBrown
In-Reply-To: <b443b3a1-ecc9-8238-1b3d-90870a25c793@suse.de>
On Thu, Feb 16, 2017 at 8:08 PM, Johannes Thumshirn <jthumshirn@suse.de> wrote:
> On 02/16/2017 12:45 PM, Ming Lei wrote:
>> MD need this helper to remove the last added page, so introduce
>> it.
>>
>> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
>> ---
>> block/bio.c | 23 +++++++++++++++++++++++
>> include/linux/bio.h | 1 +
>> 2 files changed, 24 insertions(+)
>>
>> diff --git a/block/bio.c b/block/bio.c
>> index 5eec5e08417f..0ce7ffcd7939 100644
>> --- a/block/bio.c
>> +++ b/block/bio.c
>> @@ -837,6 +837,29 @@ int bio_add_pc_page(struct request_queue *q, struct bio *bio, struct page
>> EXPORT_SYMBOL(bio_add_pc_page);
>>
>> /**
>> + * bio_remove_last_page - remove the last added page
>> + * @bio: destination bio
>> + *
>> + * Attempt to remove the last added page from the bio_vec maplist.
>> + */
>> +void bio_remove_last_page(struct bio *bio)
>> +{
>> + /*
>> + * cloned bio must not modify vec list
>> + */
>> + if (WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)))
>> + return;
>> +
>> + if (bio->bi_vcnt > 0) {
>
> In patch 1 you introduce bio_segments_all() with the log message 'So
> that we can replace the direct access to .bi_vcnt.' Here you introduce a
> new direct access to it (plus the duplicated WARN_ON_ONCE()).
>
> Maybe use the helper directly here (I admit I haven't gone through the
> whole series yet, so I can't see if the change is made later).
Firstly MD does need one helper to remove the last added page, as you
can see there are three such uses in patch3.
Secondly both the two helpers will be changed once multipage bvec
is supported, that means we have to change MD too after multipage bvec
if just using bio_segments_all() to replace .bi_vcnt for removing
the last added page.
Thanks,
Ming Lei
^ permalink raw reply
* Re: [PATCH 13/17] md: raid1: use bio_segments_all()
From: Johannes Thumshirn @ 2017-02-16 12:35 UTC (permalink / raw)
To: Ming Lei, Shaohua Li, Jens Axboe, linux-kernel, linux-raid,
linux-block, Christoph Hellwig, NeilBrown
In-Reply-To: <1487245547-24384-14-git-send-email-tom.leiming@gmail.com>
On 02/16/2017 12:45 PM, Ming Lei wrote:
> @@ -998,7 +998,8 @@ static void alloc_behind_pages(struct bio *bio, struct r1bio *r1_bio)
> {
> int i;
> struct bio_vec *bvec;
> - struct bio_vec *bvecs = kzalloc(bio->bi_vcnt * sizeof(struct bio_vec),
> + unsigned vcnt = bio_segments_all(bio);
> + struct bio_vec *bvecs = kzalloc(vcnt * sizeof(struct bio_vec),
> GFP_NOIO);
Maybe use kcalloc() instead of kzalloc() with a multiplication.
Byte,
Johannes
--
Johannes Thumshirn Storage
jthumshirn@suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: Felix Imend�rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N�rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
^ permalink raw reply
* Re: [PATCH 02/17] block: introduce bio_remove_last_page()
From: Johannes Thumshirn @ 2017-02-16 12:08 UTC (permalink / raw)
To: Ming Lei, Shaohua Li, Jens Axboe, linux-kernel, linux-raid,
linux-block, Christoph Hellwig, NeilBrown
In-Reply-To: <1487245547-24384-3-git-send-email-tom.leiming@gmail.com>
On 02/16/2017 12:45 PM, Ming Lei wrote:
> MD need this helper to remove the last added page, so introduce
> it.
>
> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
> ---
> block/bio.c | 23 +++++++++++++++++++++++
> include/linux/bio.h | 1 +
> 2 files changed, 24 insertions(+)
>
> diff --git a/block/bio.c b/block/bio.c
> index 5eec5e08417f..0ce7ffcd7939 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -837,6 +837,29 @@ int bio_add_pc_page(struct request_queue *q, struct bio *bio, struct page
> EXPORT_SYMBOL(bio_add_pc_page);
>
> /**
> + * bio_remove_last_page - remove the last added page
> + * @bio: destination bio
> + *
> + * Attempt to remove the last added page from the bio_vec maplist.
> + */
> +void bio_remove_last_page(struct bio *bio)
> +{
> + /*
> + * cloned bio must not modify vec list
> + */
> + if (WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)))
> + return;
> +
> + if (bio->bi_vcnt > 0) {
In patch 1 you introduce bio_segments_all() with the log message 'So
that we can replace the direct access to .bi_vcnt.' Here you introduce a
new direct access to it (plus the duplicated WARN_ON_ONCE()).
Maybe use the helper directly here (I admit I haven't gone through the
whole series yet, so I can't see if the change is made later).
Byte,
Johannes
--
Johannes Thumshirn Storage
jthumshirn@suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: Felix Imend�rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N�rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
^ permalink raw reply
* [PATCH 16/17] md: raid10: avoid direct access to bvec table in reshape_request
From: Ming Lei @ 2017-02-16 11:45 UTC (permalink / raw)
To: Shaohua Li, Jens Axboe, linux-kernel, linux-raid, linux-block,
Christoph Hellwig, NeilBrown
Cc: Ming Lei
In-Reply-To: <1487245547-24384-1-git-send-email-tom.leiming@gmail.com>
The cost is 128bytes(8*16) stack space in kernel thread context, and
just use the bio helper to retrieve pages from bio.
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
drivers/md/raid10.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index c7d2f73565d9..5016ce8163c3 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -4383,6 +4383,8 @@ static sector_t reshape_request(struct mddev *mddev, sector_t sector_nr,
struct bio *blist;
struct bio *bio, *read_bio;
int sectors_done = 0;
+ struct bio_vec *bvl;
+ struct page *pages[RESYNC_PAGES];
if (sector_nr == 0) {
/* If restarting in the middle, skip the initial sectors */
@@ -4546,9 +4548,12 @@ static sector_t reshape_request(struct mddev *mddev, sector_t sector_nr,
/* Now add as many pages as possible to all of these bios. */
+ bio_for_each_segment_all(bvl, r10_bio->devs[0].bio, s)
+ pages[s] = bvl->bv_page;
+
nr_sectors = 0;
for (s = 0 ; s < max_sectors; s += PAGE_SIZE >> 9) {
- struct page *page = r10_bio->devs[0].bio->bi_io_vec[s/(PAGE_SIZE>>9)].bv_page;
+ struct page *page = pages[s / (PAGE_SIZE >> 9)];
int len = (max_sectors - s) << 9;
if (len > PAGE_SIZE)
len = PAGE_SIZE;
--
2.7.4
^ permalink raw reply related
* [PATCH 17/17] md: raid10: avoid direct access to bvec table in handle_reshape_read_error
From: Ming Lei @ 2017-02-16 11:45 UTC (permalink / raw)
To: Shaohua Li, Jens Axboe, linux-kernel, linux-raid, linux-block,
Christoph Hellwig, NeilBrown
Cc: Ming Lei
In-Reply-To: <1487245547-24384-1-git-send-email-tom.leiming@gmail.com>
The cost is 128bytes(8*16) stack space in kernel thread context, and
just use the bio helper to retrieve pages from bio.
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
drivers/md/raid10.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 5016ce8163c3..d9d7c79a3bd4 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -4689,7 +4689,15 @@ static int handle_reshape_read_error(struct mddev *mddev,
struct r10bio *r10b = &on_stack.r10_bio;
int slot = 0;
int idx = 0;
- struct bio_vec *bvec = r10_bio->master_bio->bi_io_vec;
+ struct bio_vec *bvl;
+ struct page *pages[RESYNC_PAGES];
+
+ /*
+ * This bio is allocated in reshape_request(), and size
+ * is still RESYNC_PAGES
+ */
+ bio_for_each_segment_all(bvl, r10_bio->master_bio, idx)
+ pages[idx] = bvl->bv_page;
r10b->sector = r10_bio->sector;
__raid10_find_phys(&conf->prev, r10b);
@@ -4718,7 +4726,7 @@ static int handle_reshape_read_error(struct mddev *mddev,
success = sync_page_io(rdev,
addr,
s << 9,
- bvec[idx].bv_page,
+ pages[idx],
REQ_OP_READ, 0, false);
rdev_dec_pending(rdev, mddev);
rcu_read_lock();
--
2.7.4
^ permalink raw reply related
* [PATCH 15/17] md: raid10: avoid direct access to bvec table in fix_recovery_read_error
From: Ming Lei @ 2017-02-16 11:45 UTC (permalink / raw)
To: Shaohua Li, Jens Axboe, linux-kernel, linux-raid, linux-block,
Christoph Hellwig, NeilBrown
Cc: Ming Lei
In-Reply-To: <1487245547-24384-1-git-send-email-tom.leiming@gmail.com>
The cost is 128bytes(8*16) stack space in kernel thread context, and just
use the bio helper to retrieve pages from bio.
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
drivers/md/raid10.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 69fe2a3cef89..c7d2f73565d9 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -2184,6 +2184,11 @@ static void fix_recovery_read_error(struct r10bio *r10_bio)
int idx = 0;
int dr = r10_bio->devs[0].devnum;
int dw = r10_bio->devs[1].devnum;
+ struct bio_vec *bvl;
+ struct page *pages[RESYNC_PAGES];
+
+ bio_for_each_segment_all(bvl, bio, idx)
+ pages[idx] = bvl->bv_page;
while (sectors) {
int s = sectors;
@@ -2199,7 +2204,7 @@ static void fix_recovery_read_error(struct r10bio *r10_bio)
ok = sync_page_io(rdev,
addr,
s << 9,
- bio->bi_io_vec[idx].bv_page,
+ pages[idx],
REQ_OP_READ, 0, false);
if (ok) {
rdev = conf->mirrors[dw].rdev;
@@ -2207,7 +2212,7 @@ static void fix_recovery_read_error(struct r10bio *r10_bio)
ok = sync_page_io(rdev,
addr,
s << 9,
- bio->bi_io_vec[idx].bv_page,
+ pages[idx],
REQ_OP_WRITE, 0, false);
if (!ok) {
set_bit(WriteErrorSeen, &rdev->flags);
--
2.7.4
^ permalink raw reply related
* [PATCH 10/17] md: raid1: remove direct access to bvec table in fix_sync_read_error
From: Ming Lei @ 2017-02-16 11:45 UTC (permalink / raw)
To: Shaohua Li, Jens Axboe, linux-kernel, linux-raid, linux-block,
Christoph Hellwig, NeilBrown
Cc: Ming Lei
In-Reply-To: <1487245547-24384-1-git-send-email-tom.leiming@gmail.com>
This patch uses a stack variable to hold the pages in bio, so
that we can remove direct access to bvec table in fix_sync_read_error().
This 16*8 stack variable is just fine for kernel thread context.
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
drivers/md/raid1.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 1dd6b2760fba..02ee8542295d 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1874,6 +1874,16 @@ static int fix_sync_read_error(struct r1bio *r1_bio)
int sectors = r1_bio->sectors;
int idx = 0;
struct md_rdev *rdev;
+ struct bio_vec *bvl;
+ int i;
+ struct page *pages[RESYNC_PAGES];
+
+ /*
+ * bio for read_disk is filled up, so we can use
+ * bio_for_each_segment_all() to retrieve all pages.
+ */
+ bio_for_each_segment_all(bvl, bio, i)
+ pages[i] = bvl->bv_page;
rdev = conf->mirrors[r1_bio->read_disk].rdev;
if (test_bit(FailFast, &rdev->flags)) {
@@ -1903,7 +1913,7 @@ static int fix_sync_read_error(struct r1bio *r1_bio)
*/
rdev = conf->mirrors[d].rdev;
if (sync_page_io(rdev, sect, s<<9,
- bio->bi_io_vec[idx].bv_page,
+ pages[idx],
REQ_OP_READ, 0, false)) {
success = 1;
break;
@@ -1958,7 +1968,7 @@ static int fix_sync_read_error(struct r1bio *r1_bio)
continue;
rdev = conf->mirrors[d].rdev;
if (r1_sync_page_io(rdev, sect, s,
- bio->bi_io_vec[idx].bv_page,
+ pages[idx],
WRITE) == 0) {
r1_bio->bios[d]->bi_end_io = NULL;
rdev_dec_pending(rdev, mddev);
@@ -1973,7 +1983,7 @@ static int fix_sync_read_error(struct r1bio *r1_bio)
continue;
rdev = conf->mirrors[d].rdev;
if (r1_sync_page_io(rdev, sect, s,
- bio->bi_io_vec[idx].bv_page,
+ pages[idx],
READ) != 0)
atomic_add(s, &rdev->corrected_errors);
}
--
2.7.4
^ permalink raw reply related
* [PATCH 14/17] md: raid10: avoid direct access to bvec table in sync_request_write()
From: Ming Lei @ 2017-02-16 11:45 UTC (permalink / raw)
To: Shaohua Li, Jens Axboe, linux-kernel, linux-raid, linux-block,
Christoph Hellwig, NeilBrown
Cc: Ming Lei
In-Reply-To: <1487245547-24384-1-git-send-email-tom.leiming@gmail.com>
The cost is 256bytes(8*16*2) stack space, and just use the bio
helper to retrieve pages from bio.
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
drivers/md/raid10.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 5c698f3d3083..69fe2a3cef89 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -2036,6 +2036,8 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
int i, first;
struct bio *tbio, *fbio;
int vcnt;
+ struct bio_vec *bvl;
+ struct page *fbio_pages[RESYNC_PAGES], *tbio_pages[RESYNC_PAGES];
atomic_set(&r10_bio->remaining, 1);
@@ -2052,6 +2054,10 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
fbio->bi_iter.bi_size = r10_bio->sectors << 9;
fbio->bi_iter.bi_idx = 0;
+ /* the bio has been filled up in raid10_sync_request */
+ bio_for_each_segment_all(bvl, fbio, i)
+ fbio_pages[i] = bvl->bv_page;
+
vcnt = (r10_bio->sectors + (PAGE_SIZE >> 9) - 1) >> (PAGE_SHIFT - 9);
/* now find blocks with errors */
for (i=0 ; i < conf->copies ; i++) {
@@ -2072,12 +2078,17 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
* All vec entries are PAGE_SIZE;
*/
int sectors = r10_bio->sectors;
+
+ /* the bio has been filled up in raid10_sync_request */
+ bio_for_each_segment_all(bvl, tbio, j)
+ tbio_pages[j] = bvl->bv_page;
+
for (j = 0; j < vcnt; j++) {
int len = PAGE_SIZE;
if (sectors < (len / 512))
len = sectors * 512;
- if (memcmp(page_address(fbio->bi_io_vec[j].bv_page),
- page_address(tbio->bi_io_vec[j].bv_page),
+ if (memcmp(page_address(fbio_pages[j]),
+ page_address(tbio_pages[j]),
len))
break;
sectors -= len/512;
--
2.7.4
^ 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