* Re: [PATCH 08/16] mmc: core: do away with is_new_req
From: Bartlomiej Zolnierkiewicz @ 2017-02-28 16:11 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-mmc, Ulf Hansson, Adrian Hunter, Paolo Valente,
Chunyan Zhang, Baolin Wang, linux-block, Jens Axboe,
Christoph Hellwig, Arnd Bergmann
In-Reply-To: <20170209153403.9730-9-linus.walleij@linaro.org>
On Thursday, February 09, 2017 04:33:55 PM Linus Walleij wrote:
> The host context member "is_new_req" is only assigned values,
> never checked. Delete it.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
* Re: [PATCH 09/16] mmc: core: kill off the context info
From: Bartlomiej Zolnierkiewicz @ 2017-02-28 16:11 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-mmc, Ulf Hansson, Adrian Hunter, Paolo Valente,
Chunyan Zhang, Baolin Wang, linux-block, Jens Axboe,
Christoph Hellwig, Arnd Bergmann
In-Reply-To: <20170209153403.9730-10-linus.walleij@linaro.org>
On Thursday, February 09, 2017 04:33:56 PM Linus Walleij wrote:
> The last member of the context info: is_waiting_last_req is
> just assigned values, never checked. Delete that and the whole
> context info as a result.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
* Re: [PATCH 10/16] mmc: queue: simplify queue logic
From: Bartlomiej Zolnierkiewicz @ 2017-02-28 16:11 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-mmc, Ulf Hansson, Adrian Hunter, Paolo Valente,
Chunyan Zhang, Baolin Wang, linux-block, Jens Axboe,
Christoph Hellwig, Arnd Bergmann
In-Reply-To: <20170209153403.9730-11-linus.walleij@linaro.org>
On Thursday, February 09, 2017 04:33:57 PM Linus Walleij wrote:
> The if() statment checking if there is no current or previous
> request is now just looking ahead at something that will be
> concluded a few lines below. Simplify the logic by moving the
> assignment of .asleep.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
* Re: [PATCH 07/13] bdi: Move removal from bdi->wb_list into wb_shutdown()
From: Tejun Heo @ 2017-02-28 16:14 UTC (permalink / raw)
To: Jan Kara
Cc: Jens Axboe, linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, Lekshmi Pillai, NeilBrown, Omar Sandoval
In-Reply-To: <20170221170958.21845-8-jack@suse.cz>
On Tue, Feb 21, 2017 at 06:09:52PM +0100, Jan Kara wrote:
> Currently the removal from bdi->wb_list happens directly in
> cgwb_release_workfn(). Move it to wb_shutdown() which is functionally
> equivalent and more logical (the list gets only used for distributing
> writeback works among bdi_writeback structures). It will also allow us
> to simplify writeback shutdown in cgwb_bdi_destroy().
>
> Signed-off-by: Jan Kara <jack@suse.cz>
Acked-by: Tejun Heo <tj@kernel.org>
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH 0/3] Separate zone requests from medium access requests
From: Bart Van Assche @ 2017-02-28 16:16 UTC (permalink / raw)
To: hch@lst.de, Damien Le Moal
Cc: linux-scsi@vger.kernel.org, hare@suse.de,
linux-block@vger.kernel.org, MPT-FusionLinux.pdl@broadcom.com,
martin.petersen@oracle.com, axboe@kernel.dk
In-Reply-To: <20170228160259.GA22755@lst.de>
On Tue, 2017-02-28 at 17:02 +0100, Christoph Hellwig wrote:
> I don't really like this too much - this is too many SCSI specifics
> for the block layer to care. Maybe using bios for the zone ops was a
> mistake after all, and we should just have operations in struct block_dev=
ice
> instead..
blk_rq_accesses_medium() has nothing to do with SCSI. It is a reintroductio=
n
of what REQ_TYPE_FS stood for before ZBC was introduced but with another na=
me.
BTW, I think that all blk_rq_is_passthrough() callers have to be reviewed t=
o
see whether or not !blk_rq_accesses_medium() is perhaps what was intended.
Bart.=
^ permalink raw reply
* Re: [PATCH 08/13] bdi: Shutdown writeback on all cgwbs in cgwb_bdi_destroy()
From: Tejun Heo @ 2017-02-28 16:44 UTC (permalink / raw)
To: Jan Kara
Cc: Jens Axboe, linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, Lekshmi Pillai, NeilBrown, Omar Sandoval
In-Reply-To: <20170221170958.21845-9-jack@suse.cz>
Hello,
On Tue, Feb 21, 2017 at 06:09:53PM +0100, Jan Kara wrote:
> Currently we waited for all cgwbs to get freed in cgwb_bdi_destroy()
> which also means that writeback has been shutdown on them. Since this
> wait is going away, directly shutdown writeback on cgwbs from
> cgwb_bdi_destroy() to avoid live writeback structures after
> bdi_unregister() has finished.
Hmmm... the only thing which is a bit bothering is that after removing
the shutdown from release_workfn this would make the final flushing
sequential, one cgwb at a time, which in certain cases can take an
unnecessarily long time. It's not a correctness issue tho and if it
becomes a problem we can deal with it by splitting wb_shutdown() into
two pieces - the unregistration / issuing of the work items, and the
flushing of those. Other than that, looks good to me.
Acked-by: Tejun Heo <tj@kernel.org>
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH 09/13] bdi: Do not wait for cgwbs release in bdi_unregister()
From: Tejun Heo @ 2017-02-28 16:51 UTC (permalink / raw)
To: Jan Kara
Cc: Jens Axboe, linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, Lekshmi Pillai, NeilBrown, Omar Sandoval
In-Reply-To: <20170221170958.21845-10-jack@suse.cz>
Hello,
On Tue, Feb 21, 2017 at 06:09:54PM +0100, Jan Kara wrote:
> @@ -726,14 +718,6 @@ static void cgwb_bdi_destroy(struct backing_dev_info *bdi)
> }
>
> spin_unlock_irq(&cgwb_lock);
> -
> - /*
> - * All cgwb's and their congested states must be shutdown and
> - * released before returning. Drain the usage counter to wait for
> - * all cgwb's and cgwb_congested's ever created on @bdi.
> - */
> - atomic_dec(&bdi->usage_cnt);
> - wait_event(cgwb_release_wait, !atomic_read(&bdi->usage_cnt));
> }
Hmm... I'm not sure about wb_shutdown() synchronization. If you look
at the function, it's allowed to be called multiple times but doesn't
synchronize the end of the operation. With usage_cnt, it was okay
because cgwb_bdi_destroy() would have waited until everything is
finished via usage_cnt, but with that gone, we can have a race like
the following.
A B
a cgroup gets removed
a cgwb starts to get destroyed
it starts wb_shutdown()
bdi starts getting destroyed
calls cgwb_bdi_destroy()
calls wb_shutdown() on the same cgwb
but it returns because it lost to
wb_shutdown() is still in progress A's wb_shutdown()
bdi destruction proceeds
Oops.
So, I think we need to make sure that wb_shutdown()'s are properly
synchronized from start to end to get rid of the usage_cnt waiting.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH 10/13] bdi: Rename cgwb_bdi_destroy() to cgwb_bdi_unregister()
From: Tejun Heo @ 2017-02-28 16:51 UTC (permalink / raw)
To: Jan Kara
Cc: Jens Axboe, linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, Lekshmi Pillai, NeilBrown, Omar Sandoval
In-Reply-To: <20170221170958.21845-11-jack@suse.cz>
On Tue, Feb 21, 2017 at 06:09:55PM +0100, Jan Kara wrote:
> Rename cgwb_bdi_destroy() to cgwb_bdi_unregister() as it gets called
> from bdi_unregister() which is not necessarily called from bdi_destroy()
> and thus the name is somewhat misleading.
>
> Signed-off-by: Jan Kara <jack@suse.cz>
Acked-by: Tejun Heo <tj@kernel.org>
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH 05/12] fs: add a F_IOINFO fcntl
From: Darrick J. Wong @ 2017-02-28 16:51 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-fsdevel, linux-xfs, linux-block
In-Reply-To: <20170228145737.19016-6-hch@lst.de>
On Tue, Feb 28, 2017 at 06:57:30AM -0800, Christoph Hellwig wrote:
> This fcntl can be used to query I/O parameters for the given file
> descriptor. Initially it is used for the I/O alignment and atomic
> write parameters.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/fcntl.c | 18 ++++++++++++++++++
> include/linux/fs.h | 1 +
> include/uapi/linux/fcntl.h | 16 ++++++++++++++++
> 3 files changed, 35 insertions(+)
>
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index ca5d228be7ea..248fb4cc66a6 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -241,6 +241,21 @@ static int f_getowner_uids(struct file *filp, unsigned long arg)
> }
> #endif
>
> +static int fcntl_ioinfo(struct file *file, void __user *argp)
> +{
> + struct fcntl_ioinfo fio = { 0, };
> +
> + if (file->f_op->ioinfo) {
> + int ret = file->f_op->ioinfo(file, &fio);
> + if (ret)
> + return ret;
> + }
> +
> + if (copy_to_user(argp, &fio, sizeof(fio)))
> + return -EFAULT;
> + return 0;
> +}
> +
> static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
> struct file *filp)
> {
> @@ -335,6 +350,9 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
> case F_GET_SEALS:
> err = shmem_fcntl(filp, cmd, arg);
> break;
> + case F_IOINFO:
> + err = fcntl_ioinfo(filp, (void __user *)arg);
> + break;
> default:
> break;
> }
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 2ba074328894..33b08a8c2bc3 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1680,6 +1680,7 @@ struct file_operations {
> u64);
> ssize_t (*dedupe_file_range)(struct file *, u64, u64, struct file *,
> u64);
> + int (*ioinfo)(struct file *, struct fcntl_ioinfo *);
> };
>
> struct inode_operations {
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index beed138bd359..6b0aaba7c623 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -42,6 +42,22 @@
> #define F_SEAL_WRITE 0x0008 /* prevent writes */
> /* (1U << 31) is reserved for signed error codes */
>
> +
> +#define F_IOINFO (F_LINUX_SPECIFIC_BASE + 11)
> +
> +struct fcntl_ioinfo {
> + __u16 fio_flags; /* FIO_FL_* */
> + __u16 fio_alignment; /* required I/O alignment on disk */
Hm... is fio_alignment is specified in units of bytes? If so, then
shouldn't this be a __u32 so that we can handle some weird future device
that wants, say, 1MB alignment for its atomic IO?
I'm not sure there /are/ such weird devices, and the current patchset
assumes (probably sanely) that atomic requests only have to be
lba-aligned, but otoh this is a userland field and we have plenty of
reserved space.
Though, now that I look at the XFS ioinfo patch, I guess fio_alignment
is set only for O_DIRECT files? So it's really the required alignment
for directio operations.
(Now I think I'm simply confused about this field.)
--D
> + __u32 __reserved1; /* must be zero */
> + __u64 fio_max_atomic; /* max size for atomic writes */
> + __u64 __reserved2[14];/* must be zero */
> +};
> +
> +/* supports atomic writes using O_(D)SYNC */
> +#define FIO_FL_ATOMIC_OSYNC (1 << 0)
> +/* supports atomic writes committed using fsync/fdatasync/msync */
> +#define FIO_FL_ATOMIC_FSYNC (1 << 1)
> +
> /*
> * Types of directory notifications that may be requested.
> */
> --
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 11/13] block: Fix oops in locked_inode_to_wb_and_lock_list()
From: Tejun Heo @ 2017-02-28 16:52 UTC (permalink / raw)
To: Jan Kara
Cc: Jens Axboe, linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, Lekshmi Pillai, NeilBrown, Omar Sandoval
In-Reply-To: <20170221170958.21845-12-jack@suse.cz>
On Tue, Feb 21, 2017 at 06:09:56PM +0100, Jan Kara wrote:
> When block device is closed, we call inode_detach_wb() in __blkdev_put()
> which sets inode->i_wb to NULL. That is contrary to expectations that
> inode->i_wb stays valid once set during the whole inode's lifetime and
> leads to oops in wb_get() in locked_inode_to_wb_and_lock_list() because
> inode_to_wb() returned NULL.
>
> The reason why we called inode_detach_wb() is not valid anymore though.
> BDI is guaranteed to stay along until we call bdi_put() from
> bdev_evict_inode() so we can postpone calling inode_detach_wb() to that
> moment.
>
> Also add a warning to catch if someone uses inode_detach_wb() in a
> dangerous way.
>
> Reported-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
> Signed-off-by: Jan Kara <jack@suse.cz>
Acked-by: Tejun Heo <tj@kernel.org>
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH 0/13 v2] block: Fix block device shutdown related races
From: Tejun Heo @ 2017-02-28 16:54 UTC (permalink / raw)
To: Jan Kara
Cc: Jens Axboe, linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, Lekshmi Pillai, NeilBrown, Omar Sandoval
In-Reply-To: <20170221170958.21845-1-jack@suse.cz>
Hello,
It generally looks good to me. The only worry I have is around
wb_shutdown() synchronization and if that is actually an issue it
shouldn't be too difficult to fix. The other thing which came to mind
is that the congested->__bdi sever semantics. IIRC, that one was also
to support the "bdi must go away now" behavior. As bdi is refcnted
now, I think we can probably just let cong hold onto the bdi rather
than try to sever the ref there.
Thanks a lot for working on this!
--
tejun
^ permalink raw reply
* [PATCH 0/6] Lots of NBD fixes and enhancements
From: Josef Bacik @ 2017-02-28 16:57 UTC (permalink / raw)
To: axboe, nbd-general, linux-block, kernel-team
This is kind of a big batch of patches, but they all depend on eachother so it
was hard to tease out the fixes from the enhancements without making my life
miserable.
FIXES:
nbd: set queue timeout properly
nbd: handle ERESTARTSYS properly
The ERSTARTSYS one in particular is pretty awful as we can tear down a whole
device if a userspace app has a signal pending while submitting IO. This is big
and scary but I had a debug patch on top of this to randomly induce ERESTARTSYS
to make sure it was behaving properly.
ENHANCEMENTS:
1) Handle signle path failures gracefully. This is the first step to handling
reconnects gracefully, but for right now we can easily fall back on other
connections if we happen to lose one connection.
2) Ref counting and bdev change. This is in preparation for the netlink patch
and handling reconnects and the such better.
3) Netlink interface. Trying to add the nbd-control thing was controversial,
and I realized the more I wanted to do with monitoring and stuff I would need to
use netlink for anyway. With this new interface we can easily configure and
disconnect devices, dynamically add devices if we are past our pre-allocated
limit, and dynamically find free devices to use if we don't want to try and hunt
for a device. The userspace patch to utilize this is kind of rough but can be
found in my github tree
https://github.com/josefbacik/nbd
These have been pretty well tested, but I'd like to hear any thoughts on the new
interface. Thanks,
Josef
^ permalink raw reply
* [PATCH 1/6] nbd: handle single path failures gracefully
From: Josef Bacik @ 2017-02-28 16:57 UTC (permalink / raw)
To: axboe, nbd-general, linux-block, kernel-team
In-Reply-To: <1488301031-3199-1-git-send-email-jbacik@fb.com>
Currently if we have multiple connections and one of them goes down we will tear
down the whole device. However there's no reason we need to do this as we
could have other connections that are working fine. Deal with this by keeping
track of the state of the different connections, and if we lose one we mark it
as dead and send all IO destined for that socket to one of the other healthy
sockets. Any outstanding requests that were on the dead socket will timeout and
be re-submitted properly.
Signed-off-by: Josef Bacik <jbacik@fb.com>
---
drivers/block/nbd.c | 165 +++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 124 insertions(+), 41 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 0bf2b21..7a91c8f 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -47,6 +47,8 @@ static DEFINE_MUTEX(nbd_index_mutex);
struct nbd_sock {
struct socket *sock;
struct mutex tx_lock;
+ bool dead;
+ int fallback_index;
};
#define NBD_TIMEDOUT 0
@@ -80,6 +82,7 @@ struct nbd_device {
struct nbd_cmd {
struct nbd_device *nbd;
+ int index;
struct completion send_complete;
};
@@ -188,7 +191,32 @@ 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;
- dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down connection\n");
+ 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.
+ */
+ if (nbd->socks && nbd->num_connections > 1) {
+ if (cmd->index < nbd->num_connections) {
+ struct nbd_sock *nsock =
+ nbd->socks[cmd->index];
+ mutex_lock(&nsock->tx_lock);
+ nsock->dead = true;
+ kernel_sock_shutdown(nsock->sock, SHUT_RDWR);
+ mutex_unlock(&nsock->tx_lock);
+ }
+ mutex_unlock(&nbd->config_lock);
+ blk_mq_requeue_request(req, true);
+ 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++;
@@ -294,6 +322,7 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
return -EIO;
}
+ cmd->index = index;
memset(&request, 0, sizeof(request));
request.magic = htonl(NBD_REQUEST_MAGIC);
request.type = htonl(type);
@@ -311,7 +340,7 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
if (result <= 0) {
dev_err_ratelimited(disk_to_dev(nbd->disk),
"Send control failed (result %d)\n", result);
- return -EIO;
+ return -EAGAIN;
}
if (type != NBD_CMD_WRITE)
@@ -334,7 +363,7 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
dev_err(disk_to_dev(nbd->disk),
"Send data failed (result %d)\n",
result);
- return -EIO;
+ return -EAGAIN;
}
/*
* The completion might already have come in,
@@ -361,6 +390,12 @@ static inline int sock_recv_bvec(struct nbd_device *nbd, int index,
return result;
}
+static int nbd_disconnected(struct nbd_device *nbd)
+{
+ return test_bit(NBD_DISCONNECTED, &nbd->runtime_flags) ||
+ test_bit(NBD_DISCONNECT_REQUESTED, &nbd->runtime_flags);
+}
+
/* NULL returned = something went wrong, inform userspace */
static struct nbd_cmd *nbd_read_stat(struct nbd_device *nbd, int index)
{
@@ -374,8 +409,7 @@ static struct nbd_cmd *nbd_read_stat(struct nbd_device *nbd, int index)
reply.magic = 0;
result = sock_xmit(nbd, index, 0, &reply, sizeof(reply), MSG_WAITALL);
if (result <= 0) {
- if (!test_bit(NBD_DISCONNECTED, &nbd->runtime_flags) &&
- !test_bit(NBD_DISCONNECT_REQUESTED, &nbd->runtime_flags))
+ if (!nbd_disconnected(nbd))
dev_err(disk_to_dev(nbd->disk),
"Receive control failed (result %d)\n", result);
return ERR_PTR(result);
@@ -416,8 +450,19 @@ static struct nbd_cmd *nbd_read_stat(struct nbd_device *nbd, int index)
if (result <= 0) {
dev_err(disk_to_dev(nbd->disk), "Receive data failed (result %d)\n",
result);
- req->errors++;
- return cmd;
+ /*
+ * If we've disconnected or we only have 1
+ * connection then we need to make sure we
+ * complete this request, otherwise error out
+ * and let the timeout stuff handle resubmitting
+ * this request onto another connection.
+ */
+ if (nbd_disconnected(nbd) ||
+ nbd->num_connections <= 1) {
+ req->errors++;
+ return cmd;
+ }
+ return ERR_PTR(-EIO);
}
dev_dbg(nbd_to_dev(nbd), "request %p: got %d bytes data\n",
cmd, bvec.bv_len);
@@ -462,19 +507,13 @@ static void recv_work(struct work_struct *work)
while (1) {
cmd = nbd_read_stat(nbd, args->index);
if (IS_ERR(cmd)) {
+ nbd->socks[args->index]->dead = true;
ret = PTR_ERR(cmd);
break;
}
nbd_end_request(cmd);
}
-
- /*
- * We got an error, shut everybody down if this wasn't the result of a
- * disconnect request.
- */
- if (ret && !test_bit(NBD_DISCONNECT_REQUESTED, &nbd->runtime_flags))
- sock_shutdown(nbd);
atomic_dec(&nbd->recv_threads);
wake_up(&nbd->recv_wq);
}
@@ -498,50 +537,89 @@ static void nbd_clear_que(struct nbd_device *nbd)
dev_dbg(disk_to_dev(nbd->disk), "queue cleared\n");
}
+static int find_fallback(struct nbd_device *nbd, int index)
+{
+ int new_index = -1;
+ struct nbd_sock *nsock = nbd->socks[index];
+ int fallback = nsock->fallback_index;
+
+ if (test_bit(NBD_DISCONNECTED, &nbd->runtime_flags))
+ return new_index;
+
+ if (nbd->num_connections <= 1) {
+ dev_err_ratelimited(disk_to_dev(nbd->disk),
+ "Attempted send on invalid socket\n");
+ return new_index;
+ }
+
+ if (fallback >= 0 && fallback < nbd->num_connections &&
+ !nbd->socks[fallback]->dead)
+ return fallback;
+
+ mutex_lock(&nsock->tx_lock);
+ if (nsock->fallback_index < 0 ||
+ nsock->fallback_index >= nbd->num_connections ||
+ nbd->socks[nsock->fallback_index]->dead) {
+ int i;
+ for (i = 0; i < nbd->num_connections; i++) {
+ if (i == index)
+ continue;
+ if (!nbd->socks[i]->dead) {
+ new_index = i;
+ break;
+ }
+ }
+ if (new_index < 0) {
+ mutex_unlock(&nsock->tx_lock);
+ dev_err_ratelimited(disk_to_dev(nbd->disk),
+ "Dead connection, failed to find a fallback\n");
+ return new_index;
+ }
+ nsock->fallback_index = new_index;
+ }
+ new_index = nsock->fallback_index;
+ mutex_unlock(&nsock->tx_lock);
+ return new_index;
+}
-static void nbd_handle_cmd(struct nbd_cmd *cmd, int index)
+static int nbd_handle_cmd(struct nbd_cmd *cmd, int index)
{
struct request *req = blk_mq_rq_from_pdu(cmd);
struct nbd_device *nbd = cmd->nbd;
struct nbd_sock *nsock;
+ int ret;
if (index >= nbd->num_connections) {
dev_err_ratelimited(disk_to_dev(nbd->disk),
"Attempted send on invalid socket\n");
- goto error_out;
- }
-
- if (test_bit(NBD_DISCONNECTED, &nbd->runtime_flags)) {
- dev_err_ratelimited(disk_to_dev(nbd->disk),
- "Attempted send on closed socket\n");
- goto error_out;
+ return -EINVAL;
}
-
req->errors = 0;
-
+again:
nsock = nbd->socks[index];
- mutex_lock(&nsock->tx_lock);
- if (unlikely(!nsock->sock)) {
- mutex_unlock(&nsock->tx_lock);
- dev_err_ratelimited(disk_to_dev(nbd->disk),
- "Attempted send on closed socket\n");
- goto error_out;
+ if (nsock->dead) {
+ index = find_fallback(nbd, index);
+ if (index < 0)
+ return -EIO;
+ nsock = nbd->socks[index];
}
- if (nbd_send_cmd(nbd, cmd, index) != 0) {
+ /*
+ * Some failures are related to the link going down, so anything that
+ * returns EAGAIN can be retried on a different socket.
+ */
+ mutex_lock(&nsock->tx_lock);
+ ret = nbd_send_cmd(nbd, cmd, index);
+ if (ret == -EAGAIN) {
dev_err_ratelimited(disk_to_dev(nbd->disk),
- "Request send failed\n");
- req->errors++;
- nbd_end_request(cmd);
+ "Request send failed trying another connection\n");
+ nsock->dead = true;
+ mutex_unlock(&nsock->tx_lock);
+ goto again;
}
-
mutex_unlock(&nsock->tx_lock);
- return;
-
-error_out:
- req->errors++;
- nbd_end_request(cmd);
+ return ret;
}
static int nbd_queue_rq(struct blk_mq_hw_ctx *hctx,
@@ -560,7 +638,10 @@ static int nbd_queue_rq(struct blk_mq_hw_ctx *hctx,
*/
init_completion(&cmd->send_complete);
blk_mq_start_request(bd->rq);
- nbd_handle_cmd(cmd, hctx->queue_num);
+ if (nbd_handle_cmd(cmd, hctx->queue_num) != 0) {
+ bd->rq->errors++;
+ nbd_end_request(cmd);
+ }
complete(&cmd->send_complete);
return BLK_MQ_RQ_QUEUE_OK;
@@ -596,6 +677,8 @@ static int nbd_add_socket(struct nbd_device *nbd, struct block_device *bdev,
nbd->socks = socks;
+ nsock->fallback_index = -1;
+ nsock->dead = false;
mutex_init(&nsock->tx_lock);
nsock->sock = sock;
socks[nbd->num_connections++] = nsock;
--
2.7.4
^ permalink raw reply related
* [PATCH 2/6] nbd: ref count the nbd device
From: Josef Bacik @ 2017-02-28 16:57 UTC (permalink / raw)
To: axboe, nbd-general, linux-block, kernel-team
In-Reply-To: <1488301031-3199-1-git-send-email-jbacik@fb.com>
In preparation for seamless reconnects and the netlink configuration
interface we need a way to make sure our nbd device configuration
doesn't disappear until we are finished with it. So add a ref counter,
and on the final put we do all of the cleanup work on the nbd device.
At configuration time we only allow a device to be configured if it's
ref count is currently 0.
Signed-off-by: Josef Bacik <jbacik@fb.com>
---
drivers/block/nbd.c | 210 +++++++++++++++++++++++++++++++++-------------------
1 file changed, 133 insertions(+), 77 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 7a91c8f..6760ee5 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -51,22 +51,33 @@ struct nbd_sock {
int fallback_index;
};
+struct recv_thread_args {
+ struct work_struct work;
+ struct nbd_device *nbd;
+ int index;
+};
+
#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 nbd_sock **socks;
+ atomic_t refs;
+ wait_queue_head_t socks_wq;
+ int num_connections;
+
+ struct recv_thread_args *args;
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;
@@ -101,7 +112,7 @@ 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 void nbd_put(struct nbd_device *nbd);
static inline struct device *nbd_to_dev(struct nbd_device *nbd)
{
@@ -125,6 +136,25 @@ static const char *nbdcmd_to_ascii(int cmd)
return "invalid";
}
+static ssize_t pid_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct gendisk *disk = dev_to_disk(dev);
+ struct nbd_device *nbd = (struct nbd_device *)disk->private_data;
+
+ return sprintf(buf, "%d\n", task_pid_nr(nbd->task_recv));
+}
+
+static struct device_attribute pid_attr = {
+ .attr = { .name = "pid", .mode = S_IRUGO},
+ .show = pid_show,
+};
+
+static int nbd_get_unless_zero(struct nbd_device *nbd)
+{
+ return atomic_add_unless(&nbd->refs, 1, 0);
+}
+
static int nbd_size_clear(struct nbd_device *nbd, struct block_device *bdev)
{
bd_set_size(bdev, 0);
@@ -181,6 +211,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");
}
@@ -191,10 +222,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_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.
@@ -208,21 +243,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_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_put(nbd);
+
return BLK_EH_HANDLED;
}
@@ -474,26 +507,6 @@ static struct nbd_cmd *nbd_read_stat(struct nbd_device *nbd, int index)
return cmd;
}
-static ssize_t pid_show(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- struct gendisk *disk = dev_to_disk(dev);
- struct nbd_device *nbd = (struct nbd_device *)disk->private_data;
-
- return sprintf(buf, "%d\n", task_pid_nr(nbd->task_recv));
-}
-
-static struct device_attribute pid_attr = {
- .attr = { .name = "pid", .mode = S_IRUGO},
- .show = pid_show,
-};
-
-struct recv_thread_args {
- struct work_struct work;
- struct nbd_device *nbd;
- int index;
-};
-
static void recv_work(struct work_struct *work)
{
struct recv_thread_args *args = container_of(work,
@@ -516,6 +529,7 @@ static void recv_work(struct work_struct *work)
}
atomic_dec(&nbd->recv_threads);
wake_up(&nbd->recv_wq);
+ nbd_put(nbd);
}
static void nbd_clear_req(struct request *req, void *data, bool reserved)
@@ -589,9 +603,16 @@ static int nbd_handle_cmd(struct nbd_cmd *cmd, int index)
struct nbd_sock *nsock;
int ret;
+ if (!nbd_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_put(nbd);
return -EINVAL;
}
req->errors = 0;
@@ -599,8 +620,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_put(nbd);
return -EIO;
+ }
nsock = nbd->socks[index];
}
@@ -618,7 +641,7 @@ static int nbd_handle_cmd(struct nbd_cmd *cmd, int index)
goto again;
}
mutex_unlock(&nsock->tx_lock);
-
+ nbd_put(nbd);
return ret;
}
@@ -659,21 +682,27 @@ static int nbd_add_socket(struct nbd_device *nbd, struct block_device *bdev,
if (!sock)
return err;
- if (!nbd->task_setup)
+ err = -EINVAL;
+ if (!nbd->task_setup && !atomic_cmpxchg(&nbd->refs, 0, 1)) {
nbd->task_setup = current;
+ set_bit(NBD_HAS_SOCKS_REF, &nbd->runtime_flags);
+ try_module_get(THIS_MODULE);
+ }
+
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;
@@ -685,7 +714,9 @@ 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:
+ return err;
}
/* Reset all properties of an NBD device */
@@ -697,6 +728,7 @@ static void nbd_reset(struct nbd_device *nbd)
set_capacity(nbd->disk, 0);
nbd->flags = 0;
nbd->tag_set.timeout = 0;
+ nbd->args = NULL;
queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
}
@@ -741,20 +773,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_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_put(nbd);
return 0;
}
@@ -764,49 +793,74 @@ 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_put(nbd);
return 0;
}
+static void nbd_put(struct nbd_device *nbd)
+{
+ if (atomic_dec_and_test(&nbd->refs)) {
+ struct block_device *bdev;
+
+ bdev = bdget_disk(nbd->disk, 0);
+ if (!bdev)
+ return;
+
+ mutex_lock(&nbd->config_lock);
+ nbd_dev_dbg_close(nbd);
+ nbd_size_clear(nbd, bdev);
+ device_remove_file(disk_to_dev(nbd->disk), &pid_attr);
+ nbd->task_recv = NULL;
+ nbd_clear_sock(nbd, bdev);
+ 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;
+ }
+ kfree(nbd->args);
+ nbd_reset(nbd);
+ mutex_unlock(&nbd->config_lock);
+ bdput(bdev);
+ module_put(THIS_MODULE);
+ }
+}
+
static int nbd_start_device(struct nbd_device *nbd, struct block_device *bdev)
{
struct recv_thread_args *args;
int num_connections = nbd->num_connections;
int error = 0, i;
- if (nbd->task_recv)
- return -EBUSY;
- if (!nbd->socks)
+ if (!nbd_get_unless_zero(nbd))
return -EINVAL;
+ if (nbd->task_recv) {
+ error = -EBUSY;
+ goto out;
+ }
+ if (!nbd->socks) {
+ error = -EINVAL;
+ goto out;
+ }
+
if (num_connections > 1 &&
!(nbd->flags & NBD_FLAG_CAN_MULTI_CONN)) {
dev_err(disk_to_dev(nbd->disk), "server does not support multiple connections per device.\n");
error = -EINVAL;
- goto out_err;
+ goto out;
}
- 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) {
error = -ENOMEM;
- goto out_err;
+ goto out;
}
+ nbd->args = args;
nbd->task_recv = current;
mutex_unlock(&nbd->config_lock);
@@ -815,7 +869,7 @@ static int nbd_start_device(struct nbd_device *nbd, struct block_device *bdev)
error = device_create_file(disk_to_dev(nbd->disk), &pid_attr);
if (error) {
dev_err(disk_to_dev(nbd->disk), "device_create_file failed!\n");
- goto out_recv;
+ goto out;
}
nbd_size_update(nbd, bdev);
@@ -824,32 +878,26 @@ 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->refs);
INIT_WORK(&args[i].work, recv_work);
args[i].nbd = nbd;
args[i].index = i;
queue_work(recv_workqueue, &args[i].work);
}
- wait_event_interruptible(nbd->recv_wq,
- atomic_read(&nbd->recv_threads) == 0);
+ error = wait_event_interruptible(nbd->recv_wq,
+ atomic_read(&nbd->recv_threads) == 0);
+ if (error)
+ sock_shutdown(nbd);
for (i = 0; i < num_connections; i++)
flush_work(&args[i].work);
- nbd_dev_dbg_close(nbd);
- nbd_size_clear(nbd, bdev);
- device_remove_file(disk_to_dev(nbd->disk), &pid_attr);
-out_recv:
mutex_lock(&nbd->config_lock);
- nbd->task_recv = NULL;
-out_err:
- clear_bit(NBD_RUNNING, &nbd->runtime_flags);
- nbd_clear_sock(nbd, bdev);
-
+out:
/* user requested, ignore socket errors */
if (test_bit(NBD_DISCONNECT_REQUESTED, &nbd->runtime_flags))
error = 0;
if (test_bit(NBD_TIMEDOUT, &nbd->runtime_flags))
error = -ETIMEDOUT;
-
- nbd_reset(nbd);
+ nbd_put(nbd);
return error;
}
@@ -905,16 +953,21 @@ static int nbd_ioctl(struct block_device *bdev, fmode_t mode,
{
struct nbd_device *nbd = bdev->bd_disk->private_data;
int error;
+ bool need_put = false;
if (!capable(CAP_SYS_ADMIN))
return -EPERM;
BUG_ON(nbd->magic != NBD_MAGIC);
+ /* This is to keep us from doing the final put under the config_lock. */
+ if (nbd_get_unless_zero(nbd))
+ need_put = true;
mutex_lock(&nbd->config_lock);
error = __nbd_ioctl(bdev, nbd, cmd, arg);
mutex_unlock(&nbd->config_lock);
-
+ if (need_put)
+ nbd_put(nbd);
return error;
}
@@ -1141,12 +1194,15 @@ static int nbd_dev_add(int index)
nbd->magic = NBD_MAGIC;
mutex_init(&nbd->config_lock);
+ atomic_set(&nbd->refs, 0);
+ nbd->args = NULL;
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 3/6] nbd: stop using the bdev everywhere
From: Josef Bacik @ 2017-02-28 16:57 UTC (permalink / raw)
To: axboe, nbd-general, linux-block, kernel-team
In-Reply-To: <1488301031-3199-1-git-send-email-jbacik@fb.com>
In preparation for the upcoming netlink interface we need to not rely on
already having the bdev for the NBD device we are doing operations on.
Instead of passing the bdev around, just use it in places where we know
we already have the bdev.
Signed-off-by: Josef Bacik <jbacik@fb.com>
---
drivers/block/nbd.c | 93 ++++++++++++++++++++++-------------------------------
1 file changed, 39 insertions(+), 54 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 6760ee5..059c80a 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -119,11 +119,6 @@ static inline struct device *nbd_to_dev(struct nbd_device *nbd)
return disk_to_dev(nbd->disk);
}
-static bool nbd_is_connected(struct nbd_device *nbd)
-{
- return !!nbd->task_recv;
-}
-
static const char *nbdcmd_to_ascii(int cmd)
{
switch (cmd) {
@@ -155,31 +150,26 @@ static int nbd_get_unless_zero(struct nbd_device *nbd)
return atomic_add_unless(&nbd->refs, 1, 0);
}
-static int nbd_size_clear(struct nbd_device *nbd, struct block_device *bdev)
+static void nbd_size_clear(struct nbd_device *nbd)
{
- bd_set_size(bdev, 0);
set_capacity(nbd->disk, 0);
kobject_uevent(&nbd_to_dev(nbd)->kobj, KOBJ_CHANGE);
-
- return 0;
}
-static void nbd_size_update(struct nbd_device *nbd, struct block_device *bdev)
+static void nbd_size_update(struct nbd_device *nbd)
{
blk_queue_logical_block_size(nbd->disk->queue, nbd->blksize);
blk_queue_physical_block_size(nbd->disk->queue, nbd->blksize);
- bd_set_size(bdev, nbd->bytesize);
set_capacity(nbd->disk, nbd->bytesize >> 9);
kobject_uevent(&nbd_to_dev(nbd)->kobj, KOBJ_CHANGE);
}
-static void nbd_size_set(struct nbd_device *nbd, struct block_device *bdev,
- loff_t blocksize, loff_t nr_blocks)
+static void nbd_size_set(struct nbd_device *nbd, loff_t blocksize,
+ loff_t nr_blocks)
{
nbd->blksize = blocksize;
nbd->bytesize = blocksize * nr_blocks;
- if (nbd_is_connected(nbd))
- nbd_size_update(nbd, bdev);
+ nbd_size_update(nbd);
}
static void nbd_end_request(struct nbd_cmd *cmd)
@@ -670,8 +660,7 @@ static int nbd_queue_rq(struct blk_mq_hw_ctx *hctx,
return BLK_MQ_RQ_QUEUE_OK;
}
-static int nbd_add_socket(struct nbd_device *nbd, struct block_device *bdev,
- unsigned long arg)
+static int nbd_add_socket(struct nbd_device *nbd, unsigned long arg)
{
struct socket *sock;
struct nbd_sock **socks;
@@ -712,8 +701,6 @@ static int nbd_add_socket(struct nbd_device *nbd, struct block_device *bdev,
nsock->sock = sock;
socks[nbd->num_connections++] = nsock;
- if (max_part)
- bdev->bd_invalidated = 1;
err = 0;
out:
return err;
@@ -734,18 +721,19 @@ static void nbd_reset(struct nbd_device *nbd)
static void nbd_bdev_reset(struct block_device *bdev)
{
- set_device_ro(bdev, false);
- bdev->bd_inode->i_size = 0;
+ bd_set_size(bdev, 0);
if (max_part > 0) {
blkdev_reread_part(bdev);
bdev->bd_invalidated = 1;
}
}
-static void nbd_parse_flags(struct nbd_device *nbd, struct block_device *bdev)
+static void nbd_parse_flags(struct nbd_device *nbd)
{
if (nbd->flags & NBD_FLAG_READ_ONLY)
- set_device_ro(bdev, true);
+ set_disk_ro(nbd->disk, true);
+ else
+ set_disk_ro(nbd->disk, false);
if (nbd->flags & NBD_FLAG_SEND_TRIM)
queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
if (nbd->flags & NBD_FLAG_SEND_FLUSH)
@@ -770,16 +758,11 @@ static void send_disconnects(struct nbd_device *nbd)
}
}
-static int nbd_disconnect(struct nbd_device *nbd, struct block_device *bdev)
+static int nbd_disconnect(struct nbd_device *nbd)
{
dev_info(disk_to_dev(nbd->disk), "NBD_DISCONNECT\n");
if (!nbd_get_unless_zero(nbd))
return -EINVAL;
-
- mutex_unlock(&nbd->config_lock);
- fsync_bdev(bdev);
- mutex_lock(&nbd->config_lock);
-
if (!test_and_set_bit(NBD_DISCONNECT_REQUESTED,
&nbd->runtime_flags))
send_disconnects(nbd);
@@ -787,33 +770,22 @@ static int nbd_disconnect(struct nbd_device *nbd, struct block_device *bdev)
return 0;
}
-static int nbd_clear_sock(struct nbd_device *nbd, struct block_device *bdev)
+static void nbd_clear_sock(struct nbd_device *nbd)
{
sock_shutdown(nbd);
nbd_clear_que(nbd);
- kill_bdev(bdev);
- nbd_bdev_reset(bdev);
nbd->task_setup = NULL;
- if (test_and_clear_bit(NBD_HAS_SOCKS_REF, &nbd->runtime_flags))
- nbd_put(nbd);
- return 0;
}
static void nbd_put(struct nbd_device *nbd)
{
if (atomic_dec_and_test(&nbd->refs)) {
- struct block_device *bdev;
-
- bdev = bdget_disk(nbd->disk, 0);
- if (!bdev)
- return;
-
mutex_lock(&nbd->config_lock);
nbd_dev_dbg_close(nbd);
- nbd_size_clear(nbd, bdev);
+ nbd_size_clear(nbd);
device_remove_file(disk_to_dev(nbd->disk), &pid_attr);
nbd->task_recv = NULL;
- nbd_clear_sock(nbd, bdev);
+ nbd_clear_sock(nbd);
if (nbd->num_connections) {
int i;
for (i = 0; i < nbd->num_connections; i++)
@@ -825,7 +797,6 @@ static void nbd_put(struct nbd_device *nbd)
kfree(nbd->args);
nbd_reset(nbd);
mutex_unlock(&nbd->config_lock);
- bdput(bdev);
module_put(THIS_MODULE);
}
}
@@ -846,7 +817,6 @@ static int nbd_start_device(struct nbd_device *nbd, struct block_device *bdev)
error = -EINVAL;
goto out;
}
-
if (num_connections > 1 &&
!(nbd->flags & NBD_FLAG_CAN_MULTI_CONN)) {
dev_err(disk_to_dev(nbd->disk), "server does not support multiple connections per device.\n");
@@ -854,6 +824,9 @@ static int nbd_start_device(struct nbd_device *nbd, struct block_device *bdev)
goto out;
}
+ if (max_part)
+ bdev->bd_invalidated = 1;
+
blk_mq_update_nr_hw_queues(&nbd->tag_set, nbd->num_connections);
args = kcalloc(num_connections, sizeof(*args), GFP_KERNEL);
if (!args) {
@@ -864,15 +837,16 @@ static int nbd_start_device(struct nbd_device *nbd, struct block_device *bdev)
nbd->task_recv = current;
mutex_unlock(&nbd->config_lock);
- nbd_parse_flags(nbd, bdev);
+ nbd_parse_flags(nbd);
error = device_create_file(disk_to_dev(nbd->disk), &pid_attr);
if (error) {
dev_err(disk_to_dev(nbd->disk), "device_create_file failed!\n");
goto out;
}
-
- nbd_size_update(nbd, bdev);
+ if (max_part)
+ bdev->bd_invalidated = 1;
+ bd_set_size(bdev, nbd->bytesize);
nbd_dev_dbg_init(nbd);
for (i = 0; i < num_connections; i++) {
@@ -901,27 +875,38 @@ static int nbd_start_device(struct nbd_device *nbd, struct block_device *bdev)
return error;
}
+static void nbd_clear_sock_ioctl(struct nbd_device *nbd,
+ struct block_device *bdev)
+{
+ nbd_clear_sock(nbd);
+ kill_bdev(bdev);
+ nbd_bdev_reset(bdev);
+ if (test_and_clear_bit(NBD_HAS_SOCKS_REF, &nbd->runtime_flags))
+ nbd_put(nbd);
+}
+
/* Must be called with config_lock held */
static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
unsigned int cmd, unsigned long arg)
{
switch (cmd) {
case NBD_DISCONNECT:
- return nbd_disconnect(nbd, bdev);
+ return nbd_disconnect(nbd);
case NBD_CLEAR_SOCK:
- return nbd_clear_sock(nbd, bdev);
+ nbd_clear_sock_ioctl(nbd, bdev);
+ return 0;
case NBD_SET_SOCK:
- return nbd_add_socket(nbd, bdev, arg);
+ return nbd_add_socket(nbd, arg);
case NBD_SET_BLKSIZE:
- nbd_size_set(nbd, bdev, arg,
+ nbd_size_set(nbd, arg,
div_s64(nbd->bytesize, arg));
return 0;
case NBD_SET_SIZE:
- nbd_size_set(nbd, bdev, nbd->blksize,
+ nbd_size_set(nbd, nbd->blksize,
div_s64(arg, nbd->blksize));
return 0;
case NBD_SET_SIZE_BLOCKS:
- nbd_size_set(nbd, bdev, nbd->blksize, arg);
+ nbd_size_set(nbd, nbd->blksize, arg);
return 0;
case NBD_SET_TIMEOUT:
nbd->tag_set.timeout = arg * HZ;
--
2.7.4
^ permalink raw reply related
* [PATCH 4/6] nbd: set queue timeout properly
From: Josef Bacik @ 2017-02-28 16:57 UTC (permalink / raw)
To: axboe, nbd-general, linux-block, kernel-team
In-Reply-To: <1488301031-3199-1-git-send-email-jbacik@fb.com>
We can't just set the timeout on the tagset, we have to set it on the
queue as it would have been setup already at this point.
Signed-off-by: Josef Bacik <jbacik@fb.com>
---
drivers/block/nbd.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 059c80a..ac5a03a 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -909,7 +909,10 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
nbd_size_set(nbd, nbd->blksize, arg);
return 0;
case NBD_SET_TIMEOUT:
- nbd->tag_set.timeout = arg * HZ;
+ if (arg) {
+ nbd->tag_set.timeout = arg * HZ;
+ blk_queue_rq_timeout(nbd->disk->queue, arg * HZ);
+ }
return 0;
case NBD_SET_FLAGS:
--
2.7.4
^ permalink raw reply related
* [PATCH 5/6] nbd: handle ERESTARTSYS properly
From: Josef Bacik @ 2017-02-28 16:57 UTC (permalink / raw)
To: axboe, nbd-general, linux-block, kernel-team
In-Reply-To: <1488301031-3199-1-git-send-email-jbacik@fb.com>
We can submit IO in a processes context, which means there can be
pending signals. This isn't a fatal error for NBD, but it does require
some finesse. If the signal happens before we transmit anything then we
are ok, just requeue the request and carry on. However if we've done a
partial transmit we can't allow anything else to be transmitted on this
socket until we transmit the remaining part of the request. Deal with
this by keeping track of how much we've sent for the current request,
and if we get an ERESTARTSYS during any part of our transmission save
the state of that request and requeue the IO. If anybody tries to
submit a request that isn't our pending request then requeue that
request until we are able to service the one that is pending.
Signed-off-by: Josef Bacik <jbacik@fb.com>
---
drivers/block/nbd.c | 138 +++++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 116 insertions(+), 22 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index ac5a03a..aba1aba 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -49,6 +49,8 @@ struct nbd_sock {
struct mutex tx_lock;
bool dead;
int fallback_index;
+ struct request *pending;
+ int sent;
};
struct recv_thread_args {
@@ -145,6 +147,13 @@ static struct device_attribute pid_attr = {
.show = pid_show,
};
+static void nbd_mark_nsock_dead(struct nbd_sock *nsock)
+{
+ nsock->dead = true;
+ nsock->pending = NULL;
+ nsock->sent = 0;
+}
+
static int nbd_get_unless_zero(struct nbd_device *nbd)
{
return atomic_add_unless(&nbd->refs, 1, 0);
@@ -200,8 +209,8 @@ static void sock_shutdown(struct nbd_device *nbd)
struct nbd_sock *nsock = nbd->socks[i];
mutex_lock(&nsock->tx_lock);
kernel_sock_shutdown(nsock->sock, SHUT_RDWR);
+ nbd_mark_nsock_dead(nsock);
mutex_unlock(&nsock->tx_lock);
- nsock->dead = true;
}
dev_warn(disk_to_dev(nbd->disk), "shutting down sockets\n");
}
@@ -229,8 +238,8 @@ static enum blk_eh_timer_return nbd_xmit_timeout(struct request *req,
struct nbd_sock *nsock =
nbd->socks[cmd->index];
mutex_lock(&nsock->tx_lock);
- nsock->dead = true;
kernel_sock_shutdown(nsock->sock, SHUT_RDWR);
+ nbd_mark_nsock_dead(nsock);
mutex_unlock(&nsock->tx_lock);
}
blk_mq_requeue_request(req, true);
@@ -253,7 +262,7 @@ static enum blk_eh_timer_return nbd_xmit_timeout(struct request *req,
* Send or receive packet.
*/
static int sock_xmit(struct nbd_device *nbd, int index, int send, void *buf,
- int size, int msg_flags)
+ int size, int msg_flags, int *sent)
{
struct socket *sock = nbd->socks[index]->sock;
int result;
@@ -292,6 +301,8 @@ static int sock_xmit(struct nbd_device *nbd, int index, int send, void *buf,
}
size -= result;
buf += result;
+ if (sent)
+ *sent += result;
} while (size > 0);
tsk_restore_flags(current, pflags, PF_MEMALLOC);
@@ -300,12 +311,28 @@ static int sock_xmit(struct nbd_device *nbd, int index, int send, void *buf,
}
static inline int sock_send_bvec(struct nbd_device *nbd, int index,
- struct bio_vec *bvec, int flags)
+ struct bio_vec *bvec, int flags, int *skip,
+ int *sent)
{
+ void *kaddr;
+ unsigned int offset = bvec->bv_offset;
+ unsigned int len = bvec->bv_len;
int result;
- void *kaddr = kmap(bvec->bv_page);
- result = sock_xmit(nbd, index, 1, kaddr + bvec->bv_offset,
- bvec->bv_len, flags);
+
+ /* If we had partially sent this request previously we need to skip over
+ * what we've already sent.
+ */
+ if (*skip) {
+ if (len <= *skip) {
+ *skip -= len;
+ return len;
+ }
+ offset += *skip;
+ len -= *skip;
+ *skip = 0;
+ }
+ kaddr = kmap(bvec->bv_page);
+ result = sock_xmit(nbd, index, 1, kaddr + offset, len, flags, sent);
kunmap(bvec->bv_page);
return result;
}
@@ -314,12 +341,14 @@ static inline int sock_send_bvec(struct nbd_device *nbd, int index,
static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
{
struct request *req = blk_mq_rq_from_pdu(cmd);
+ struct nbd_sock *nsock = nbd->socks[index];
int result;
struct nbd_request request;
unsigned long size = blk_rq_bytes(req);
struct bio *bio;
u32 type;
u32 tag = blk_mq_unique_tag(req);
+ int sent = nsock->sent, skip = 0;
switch (req_op(req)) {
case REQ_OP_DISCARD:
@@ -345,6 +374,15 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
return -EIO;
}
+ /* We did a partial send previously, and we at least sent the whole
+ * request struct, so just go and send the rest of the pages in the
+ * request.
+ */
+ if (sent && sent >= sizeof(request)) {
+ skip = sent - sizeof(request);
+ goto send_pages;
+ }
+
cmd->index = index;
memset(&request, 0, sizeof(request));
request.magic = htonl(NBD_REQUEST_MAGIC);
@@ -358,16 +396,33 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
dev_dbg(nbd_to_dev(nbd), "request %p: sending control (%s@%llu,%uB)\n",
cmd, nbdcmd_to_ascii(type),
(unsigned long long)blk_rq_pos(req) << 9, blk_rq_bytes(req));
- result = sock_xmit(nbd, index, 1, &request, sizeof(request),
- (type == NBD_CMD_WRITE) ? MSG_MORE : 0);
+
+ /* We may have only partially sent the request previously, so handle
+ * this horrible, horrible, terrifying possiblity here.
+ */
+ result = sock_xmit(nbd, index, 1, (void *)(&request) + sent,
+ sizeof(request) - sent,
+ (type == NBD_CMD_WRITE) ? MSG_MORE : 0, &sent);
if (result <= 0) {
+ if (result == -ERESTARTSYS) {
+ /* If we havne't sent anything we can just return BUSY,
+ * however if we have sent something we need to make
+ * sure we only allow this req to be sent until we are
+ * completely done.
+ */
+ if (sent) {
+ nsock->pending = req;
+ nsock->sent = sent;
+ }
+ return BLK_MQ_RQ_QUEUE_BUSY;
+ }
dev_err_ratelimited(disk_to_dev(nbd->disk),
"Send control failed (result %d)\n", result);
return -EAGAIN;
}
-
+send_pages:
if (type != NBD_CMD_WRITE)
- return 0;
+ goto out;
bio = req->bio;
while (bio) {
@@ -381,8 +436,18 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
dev_dbg(nbd_to_dev(nbd), "request %p: sending %d bytes data\n",
cmd, bvec.bv_len);
- result = sock_send_bvec(nbd, index, &bvec, flags);
+ result = sock_send_bvec(nbd, index, &bvec, flags,
+ &skip, &sent);
if (result <= 0) {
+ if (result == -ERESTARTSYS) {
+ /* We've already sent the header, we
+ * have no choice but to set pending and
+ * return BUSY.
+ */
+ nsock->pending = req;
+ nsock->sent = sent;
+ return BLK_MQ_RQ_QUEUE_BUSY;
+ }
dev_err(disk_to_dev(nbd->disk),
"Send data failed (result %d)\n",
result);
@@ -399,6 +464,9 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
}
bio = next;
}
+out:
+ nsock->pending = NULL;
+ nsock->sent = 0;
return 0;
}
@@ -408,7 +476,7 @@ static inline int sock_recv_bvec(struct nbd_device *nbd, int index,
int result;
void *kaddr = kmap(bvec->bv_page);
result = sock_xmit(nbd, index, 0, kaddr + bvec->bv_offset,
- bvec->bv_len, MSG_WAITALL);
+ bvec->bv_len, MSG_WAITALL, NULL);
kunmap(bvec->bv_page);
return result;
}
@@ -430,7 +498,8 @@ static struct nbd_cmd *nbd_read_stat(struct nbd_device *nbd, int index)
u32 tag;
reply.magic = 0;
- result = sock_xmit(nbd, index, 0, &reply, sizeof(reply), MSG_WAITALL);
+ result = sock_xmit(nbd, index, 0, &reply, sizeof(reply), MSG_WAITALL,
+ NULL);
if (result <= 0) {
if (!nbd_disconnected(nbd))
dev_err(disk_to_dev(nbd->disk),
@@ -510,7 +579,10 @@ static void recv_work(struct work_struct *work)
while (1) {
cmd = nbd_read_stat(nbd, args->index);
if (IS_ERR(cmd)) {
- nbd->socks[args->index]->dead = true;
+ struct nbd_sock *nsock = nbd->socks[args->index];
+ mutex_lock(&nsock->tx_lock);
+ nbd_mark_nsock_dead(nsock);
+ mutex_unlock(&nsock->tx_lock);
ret = PTR_ERR(cmd);
break;
}
@@ -622,14 +694,26 @@ static int nbd_handle_cmd(struct nbd_cmd *cmd, int index)
* returns EAGAIN can be retried on a different socket.
*/
mutex_lock(&nsock->tx_lock);
+
+ /* Handle the case that we have a pending request that was partially
+ * transmitted that _has_ to be serviced first. We need to call requeue
+ * here so that it gets put _after_ the request that is already on the
+ * dispatch list.
+ */
+ if (unlikely(nsock->pending && nsock->pending != req)) {
+ blk_mq_requeue_request(req, true);
+ ret = 0;
+ goto out;
+ }
ret = nbd_send_cmd(nbd, cmd, index);
if (ret == -EAGAIN) {
dev_err_ratelimited(disk_to_dev(nbd->disk),
"Request send failed trying another connection\n");
- nsock->dead = true;
+ nbd_mark_nsock_dead(nsock);
mutex_unlock(&nsock->tx_lock);
goto again;
}
+out:
mutex_unlock(&nsock->tx_lock);
nbd_put(nbd);
return ret;
@@ -639,6 +723,7 @@ static int nbd_queue_rq(struct blk_mq_hw_ctx *hctx,
const struct blk_mq_queue_data *bd)
{
struct nbd_cmd *cmd = blk_mq_rq_to_pdu(bd->rq);
+ int ret;
/*
* Since we look at the bio's to send the request over the network we
@@ -651,13 +736,20 @@ static int nbd_queue_rq(struct blk_mq_hw_ctx *hctx,
*/
init_completion(&cmd->send_complete);
blk_mq_start_request(bd->rq);
- if (nbd_handle_cmd(cmd, hctx->queue_num) != 0) {
- bd->rq->errors++;
- nbd_end_request(cmd);
- }
+
+ /* We can be called directly from the user space process, which means we
+ * could possibly have signals pending so our sendmsg will fail. In
+ * this case we need to return that we are busy, otherwise error out as
+ * appropriate.
+ */
+ ret = nbd_handle_cmd(cmd, hctx->queue_num);
+ if (ret < 0)
+ ret = BLK_MQ_RQ_QUEUE_ERROR;
+ if (!ret)
+ ret = BLK_MQ_RQ_QUEUE_OK;
complete(&cmd->send_complete);
- return BLK_MQ_RQ_QUEUE_OK;
+ return ret;
}
static int nbd_add_socket(struct nbd_device *nbd, unsigned long arg)
@@ -699,6 +791,8 @@ static int nbd_add_socket(struct nbd_device *nbd, unsigned long arg)
nsock->dead = false;
mutex_init(&nsock->tx_lock);
nsock->sock = sock;
+ nsock->pending = NULL;
+ nsock->sent = 0;
socks[nbd->num_connections++] = nsock;
err = 0;
@@ -751,7 +845,7 @@ static void send_disconnects(struct nbd_device *nbd)
request.type = htonl(NBD_CMD_DISC);
for (i = 0; i < nbd->num_connections; i++) {
- ret = sock_xmit(nbd, i, 1, &request, sizeof(request), 0);
+ ret = sock_xmit(nbd, i, 1, &request, sizeof(request), 0, NULL);
if (ret <= 0)
dev_err(disk_to_dev(nbd->disk),
"Send disconnect failed %d\n", ret);
--
2.7.4
^ permalink raw reply related
* [PATCH 6/6] nbd: add a basic netlink interface
From: Josef Bacik @ 2017-02-28 16:57 UTC (permalink / raw)
To: axboe, nbd-general, linux-block, kernel-team
In-Reply-To: <1488301031-3199-1-git-send-email-jbacik@fb.com>
The existing ioctl interface for configuring NBD devices is a bit
cumbersome and hard to extend. The other problem is we leave a
userspace app sitting in it's syscall until the device disconnects,
which is less than ideal.
This patch introduces a netlink interface for adding and disconnecting
nbd devices. This has the benefits of being easily extendable without
breaking older userspace applications, and allows us to configure a nbd
device without leaving a userspace app sitting waiting for the device to
disconnect.
With this interface we also gain the ability to configure more devices
than are preallocated at insmod time. We also have gained the ability
to not specify a particular device and be provided one for us so that
userspace doesn't need to find a free device to configure.
Signed-off-by: Josef Bacik <jbacik@fb.com>
---
drivers/block/nbd.c | 328 ++++++++++++++++++++++++++++++++++-----
include/uapi/linux/nbd-netlink.h | 69 ++++++++
2 files changed, 354 insertions(+), 43 deletions(-)
create mode 100644 include/uapi/linux/nbd-netlink.h
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index aba1aba..62a659e 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -40,6 +40,8 @@
#include <asm/types.h>
#include <linux/nbd.h>
+#include <linux/nbd-netlink.h>
+#include <net/genetlink.h>
static DEFINE_IDR(nbd_index_idr);
static DEFINE_MUTEX(nbd_index_mutex);
@@ -63,6 +65,7 @@ struct recv_thread_args {
#define NBD_DISCONNECT_REQUESTED 1
#define NBD_DISCONNECTED 2
#define NBD_HAS_SOCKS_REF 3
+#define NBD_BOUND 4
struct nbd_device {
u32 flags;
@@ -75,6 +78,7 @@ struct nbd_device {
struct recv_thread_args *args;
int magic;
+ int index;
struct blk_mq_tag_set tag_set;
@@ -115,6 +119,7 @@ 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 void nbd_put(struct nbd_device *nbd);
+static void nbd_connect_reply(struct genl_info *info, int index);
static inline struct device *nbd_to_dev(struct nbd_device *nbd)
{
@@ -752,7 +757,8 @@ static int nbd_queue_rq(struct blk_mq_hw_ctx *hctx,
return ret;
}
-static int nbd_add_socket(struct nbd_device *nbd, unsigned long arg)
+static int nbd_add_socket(struct nbd_device *nbd, unsigned long arg,
+ bool netlink)
{
struct socket *sock;
struct nbd_sock **socks;
@@ -764,13 +770,14 @@ static int nbd_add_socket(struct nbd_device *nbd, unsigned long arg)
return err;
err = -EINVAL;
- if (!nbd->task_setup && !atomic_cmpxchg(&nbd->refs, 0, 1)) {
+ if (!netlink && !nbd->task_setup &&
+ !atomic_cmpxchg(&nbd->refs, 0, 1)) {
nbd->task_setup = current;
set_bit(NBD_HAS_SOCKS_REF, &nbd->runtime_flags);
try_module_get(THIS_MODULE);
}
- if (nbd->task_setup != current) {
+ if (!netlink && nbd->task_setup != current) {
dev_err(disk_to_dev(nbd->disk),
"Device being setup by another task");
goto out;
@@ -895,52 +902,36 @@ static void nbd_put(struct nbd_device *nbd)
}
}
-static int nbd_start_device(struct nbd_device *nbd, struct block_device *bdev)
+static int nbd_start_device(struct nbd_device *nbd)
{
struct recv_thread_args *args;
int num_connections = nbd->num_connections;
int error = 0, i;
- if (!nbd_get_unless_zero(nbd))
+ if (nbd->task_recv)
+ return -EBUSY;
+ if (!nbd->socks)
return -EINVAL;
- if (nbd->task_recv) {
- error = -EBUSY;
- goto out;
- }
- if (!nbd->socks) {
- error = -EINVAL;
- goto out;
- }
if (num_connections > 1 &&
!(nbd->flags & NBD_FLAG_CAN_MULTI_CONN)) {
dev_err(disk_to_dev(nbd->disk), "server does not support multiple connections per device.\n");
- error = -EINVAL;
- goto out;
+ return -EINVAL;
}
- if (max_part)
- bdev->bd_invalidated = 1;
-
blk_mq_update_nr_hw_queues(&nbd->tag_set, nbd->num_connections);
args = kcalloc(num_connections, sizeof(*args), GFP_KERNEL);
- if (!args) {
- error = -ENOMEM;
- goto out;
- }
+ if (!args)
+ return -ENOMEM;
nbd->args = args;
nbd->task_recv = current;
- mutex_unlock(&nbd->config_lock);
nbd_parse_flags(nbd);
error = device_create_file(disk_to_dev(nbd->disk), &pid_attr);
if (error) {
dev_err(disk_to_dev(nbd->disk), "device_create_file failed!\n");
- goto out;
+ return error;
}
- if (max_part)
- bdev->bd_invalidated = 1;
- bd_set_size(bdev, nbd->bytesize);
nbd_dev_dbg_init(nbd);
for (i = 0; i < num_connections; i++) {
@@ -952,21 +943,39 @@ static int nbd_start_device(struct nbd_device *nbd, struct block_device *bdev)
args[i].index = i;
queue_work(recv_workqueue, &args[i].work);
}
- error = wait_event_interruptible(nbd->recv_wq,
+ return error;
+}
+
+static int nbd_start_device_ioctl(struct nbd_device *nbd, struct block_device *bdev)
+{
+ int ret, i;
+
+ if (!nbd_get_unless_zero(nbd))
+ return -EINVAL;
+
+ ret = nbd_start_device(nbd);
+ if (ret)
+ return ret;
+
+ bd_set_size(bdev, nbd->bytesize);
+ if (max_part)
+ bdev->bd_invalidated = 1;
+ mutex_unlock(&nbd->config_lock);
+ ret = wait_event_interruptible(nbd->recv_wq,
atomic_read(&nbd->recv_threads) == 0);
- if (error)
+ if (ret)
sock_shutdown(nbd);
- for (i = 0; i < num_connections; i++)
- flush_work(&args[i].work);
+ for (i = 0; i < nbd->num_connections; i++)
+ flush_work(&nbd->args[i].work);
mutex_lock(&nbd->config_lock);
-out:
+ bd_set_size(bdev, 0);
/* user requested, ignore socket errors */
if (test_bit(NBD_DISCONNECT_REQUESTED, &nbd->runtime_flags))
- error = 0;
+ ret = 0;
if (test_bit(NBD_TIMEDOUT, &nbd->runtime_flags))
- error = -ETIMEDOUT;
+ ret = -ETIMEDOUT;
nbd_put(nbd);
- return error;
+ return ret;
}
static void nbd_clear_sock_ioctl(struct nbd_device *nbd,
@@ -990,7 +999,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
nbd_clear_sock_ioctl(nbd, bdev);
return 0;
case NBD_SET_SOCK:
- return nbd_add_socket(nbd, arg);
+ return nbd_add_socket(nbd, arg, false);
case NBD_SET_BLKSIZE:
nbd_size_set(nbd, arg,
div_s64(nbd->bytesize, arg));
@@ -1013,7 +1022,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
nbd->flags = arg;
return 0;
case NBD_DO_IT:
- return nbd_start_device(nbd, bdev);
+ return nbd_start_device_ioctl(nbd, bdev);
case NBD_CLEAR_QUE:
/*
* This is for compatibility only. The queue is always cleared
@@ -1034,7 +1043,7 @@ static int nbd_ioctl(struct block_device *bdev, fmode_t mode,
unsigned int cmd, unsigned long arg)
{
struct nbd_device *nbd = bdev->bd_disk->private_data;
- int error;
+ int error = -EINVAL;
bool need_put = false;
if (!capable(CAP_SYS_ADMIN))
@@ -1046,7 +1055,15 @@ static int nbd_ioctl(struct block_device *bdev, fmode_t mode,
if (nbd_get_unless_zero(nbd))
need_put = true;
mutex_lock(&nbd->config_lock);
- error = __nbd_ioctl(bdev, nbd, cmd, arg);
+
+ /* Don't allow ioctl operations on a nbd device that was created with
+ * netlink, unless it's DISCONNECT or CLEAR_SOCK, which are fine.
+ */
+ if (!test_bit(NBD_BOUND, &nbd->runtime_flags) ||
+ (cmd == NBD_DISCONNECT || cmd == NBD_CLEAR_SOCK))
+ error = __nbd_ioctl(bdev, nbd, cmd, arg);
+ else
+ dev_err(nbd_to_dev(nbd), "Cannot use ioctl interface on a netlink controlled device.\n");
mutex_unlock(&nbd->config_lock);
if (need_put)
nbd_put(nbd);
@@ -1242,6 +1259,7 @@ static int nbd_dev_add(int index)
if (err < 0)
goto out_free_disk;
+ nbd->index = index;
nbd->disk = disk;
nbd->tag_set.ops = &nbd_mq_ops;
nbd->tag_set.nr_hw_queues = 1;
@@ -1301,10 +1319,228 @@ static int nbd_dev_add(int index)
return err;
}
-/*
- * And here should be modules and kernel interface
- * (Just smiley confuses emacs :-)
- */
+static int find_free_cb(int id, void *ptr, void *data)
+{
+ struct nbd_device *nbd = ptr;
+ struct nbd_device **found = data;
+
+ if (!atomic_read(&nbd->refs)) {
+ *found = nbd;
+ return 1;
+ }
+ return 0;
+}
+
+/* Netlink interface. */
+static struct nla_policy nbd_attr_policy[NBD_ATTR_MAX + 1] = {
+ [NBD_ATTR_INDEX] = { .type = NLA_U32 },
+ [NBD_ATTR_SIZE_BYTES] = { .type = NLA_U64 },
+ [NBD_ATTR_BLOCK_SIZE_BYTES] = { .type = NLA_U64 },
+ [NBD_ATTR_TIMEOUT] = { .type = NLA_U64 },
+ [NBD_ATTR_SERVER_FLAGS] = { .type = NLA_U64 },
+ [NBD_ATTR_CLIENT_FLAGS] = { .type = NLA_U64 },
+ [NBD_ATTR_SOCKETS] = { .type = NLA_NESTED},
+};
+
+static struct nla_policy nbd_sock_policy[NBD_SOCK_MAX + 1] = {
+ [NBD_SOCK_FD] = { .type = NLA_U32 },
+};
+
+static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info)
+{
+ struct nbd_device *nbd = NULL;
+ int index = -1;
+ int ret;
+
+ if (!netlink_capable(skb, CAP_SYS_ADMIN))
+ return -EPERM;
+
+ if (info->attrs[NBD_ATTR_INDEX])
+ index = nla_get_u32(info->attrs[NBD_ATTR_INDEX]);
+ if (!info->attrs[NBD_ATTR_SOCKETS]) {
+ printk(KERN_ERR "nbd: must specify at least one socket\n");
+ return -EINVAL;
+ }
+ if (!info->attrs[NBD_ATTR_SIZE_BYTES]) {
+ printk(KERN_ERR "nbd: must specify a size in bytes for the device\n");
+ return -EINVAL;
+ }
+again:
+ mutex_lock(&nbd_index_mutex);
+ if (index == -1) {
+ ret = idr_for_each(&nbd_index_idr, &find_free_cb, &nbd);
+ if (ret == 0) {
+ int new_index;
+ new_index = nbd_dev_add(-1);
+ if (new_index < 0) {
+ mutex_unlock(&nbd_index_mutex);
+ printk(KERN_ERR "nbd: failed to add new device\n");
+ return ret;
+ }
+ nbd = idr_find(&nbd_index_idr, new_index);
+ }
+ } else {
+ nbd = idr_find(&nbd_index_idr, index);
+ }
+ mutex_unlock(&nbd_index_mutex);
+ if (!nbd) {
+ printk(KERN_ERR "nbd: couldn't find device at index %d\n",
+ index);
+ return -EINVAL;
+ }
+
+ mutex_lock(&nbd->config_lock);
+ if (atomic_cmpxchg(&nbd->refs, 0, 1)) {
+ mutex_unlock(&nbd->config_lock);
+ if (index == -1)
+ goto again;
+ printk(KERN_ERR "nbd: nbd%d already in use\n", index);
+ return -EBUSY;
+ }
+ try_module_get(THIS_MODULE);
+ /*
+ * There is a very slight race where we could have just put the last
+ * reference on this device and are now unable to tear everything down,
+ * so check for NBD_BOUND or task_recv and cycle the config_mutex to
+ * allow the teardown to happen.
+ */
+ if (test_and_set_bit(NBD_BOUND, &nbd->runtime_flags) ||
+ nbd->task_recv) {
+ mutex_unlock(&nbd->config_lock);
+ mutex_lock(&nbd->config_lock);
+ set_bit(NBD_BOUND, &nbd->runtime_flags);
+ }
+
+ if (info->attrs[NBD_ATTR_SIZE_BYTES]) {
+ u64 bytes = nla_get_u64(info->attrs[NBD_ATTR_SIZE_BYTES]);
+ nbd_size_set(nbd, nbd->blksize,
+ div64_u64(bytes, nbd->blksize));
+ }
+ if (info->attrs[NBD_ATTR_BLOCK_SIZE_BYTES]) {
+ u64 bsize =
+ nla_get_u64(info->attrs[NBD_ATTR_BLOCK_SIZE_BYTES]);
+ nbd_size_set(nbd, bsize, div64_u64(nbd->bytesize, bsize));
+ }
+ if (info->attrs[NBD_ATTR_TIMEOUT]) {
+ u64 timeout = nla_get_u64(info->attrs[NBD_ATTR_TIMEOUT]);
+ nbd->tag_set.timeout = timeout * HZ;
+ }
+ if (info->attrs[NBD_ATTR_SERVER_FLAGS])
+ nbd->flags =
+ nla_get_u64(info->attrs[NBD_ATTR_SERVER_FLAGS]);
+ if (info->attrs[NBD_ATTR_SOCKETS]) {
+ struct nlattr *attr;
+ int rem, fd;
+
+ nla_for_each_nested(attr, info->attrs[NBD_ATTR_SOCKETS],
+ rem) {
+ struct nlattr *socks[NBD_SOCK_MAX+1];
+
+ if (nla_type(attr) != NBD_SOCK_ITEM) {
+ printk(KERN_ERR "nbd: socks must be embedded in a SOCK_ITEM attr\n");
+ ret = -EINVAL;
+ goto out;
+ }
+ ret = nla_parse_nested(socks, NBD_SOCK_MAX, attr,
+ nbd_sock_policy);
+ if (ret != 0) {
+ printk(KERN_ERR "nbd: error processing sock list\n");
+ ret = -EINVAL;
+ goto out;
+ }
+ if (!socks[NBD_SOCK_FD])
+ continue;
+ fd = (int)nla_get_u32(socks[NBD_SOCK_FD]);
+ ret = nbd_add_socket(nbd, fd, true);
+ if (ret)
+ goto out;
+ }
+ }
+ ret = nbd_start_device(nbd);
+out:
+ mutex_unlock(&nbd->config_lock);
+ if (!ret)
+ nbd_connect_reply(info, nbd->index);
+ nbd_put(nbd);
+ return ret;
+}
+
+static int nbd_genl_disconnect(struct sk_buff *skb, struct genl_info *info)
+{
+ struct nbd_device *nbd;
+ int index;
+
+ if (!netlink_capable(skb, CAP_SYS_ADMIN))
+ return -EPERM;
+
+ if (!info->attrs[NBD_ATTR_INDEX]) {
+ printk(KERN_ERR "nbd: must specify an index to disconnect\n");
+ return -EINVAL;
+ }
+ index = nla_get_u32(info->attrs[NBD_ATTR_INDEX]);
+ mutex_lock(&nbd_index_mutex);
+ nbd = idr_find(&nbd_index_idr, index);
+ mutex_unlock(&nbd_index_mutex);
+ if (!nbd) {
+ printk(KERN_ERR "nbd: couldn't find device at index %d\n",
+ index);
+ return -EINVAL;
+ }
+ if (!nbd_get_unless_zero(nbd))
+ return 0;
+ mutex_lock(&nbd->config_lock);
+ nbd_disconnect(nbd);
+ mutex_unlock(&nbd->config_lock);
+ nbd_put(nbd);
+ return 0;
+}
+
+static const struct genl_ops nbd_connect_genl_ops[] = {
+ {
+ .cmd = NBD_CMD_CONNECT,
+ .policy = nbd_attr_policy,
+ .doit = nbd_genl_connect,
+ },
+ {
+ .cmd = NBD_CMD_DISCONNECT,
+ .policy = nbd_attr_policy,
+ .doit = nbd_genl_disconnect,
+ },
+};
+
+static struct genl_family nbd_genl_family __ro_after_init = {
+ .hdrsize = 0,
+ .name = NBD_GENL_FAMILY_NAME,
+ .version = NBD_GENL_VERSION,
+ .module = THIS_MODULE,
+ .ops = nbd_connect_genl_ops,
+ .n_ops = ARRAY_SIZE(nbd_connect_genl_ops),
+ .maxattr = NBD_ATTR_MAX,
+};
+
+static void nbd_connect_reply(struct genl_info *info, int index)
+{
+ struct sk_buff *skb;
+ void *msg_head;
+ int ret;
+
+ skb = genlmsg_new(nla_total_size(sizeof(u32)), GFP_KERNEL);
+ if (!skb)
+ return;
+ msg_head = genlmsg_put_reply(skb, info, &nbd_genl_family, 0,
+ NBD_CMD_CONNECT);
+ if (!msg_head) {
+ nlmsg_free(skb);
+ return;
+ }
+ ret = nla_put_u32(skb, NBD_ATTR_INDEX, index);
+ if (ret) {
+ nlmsg_free(skb);
+ return;
+ }
+ genlmsg_end(skb, msg_head);
+ genlmsg_reply(skb, info);
+}
static int __init nbd_init(void)
{
@@ -1347,6 +1583,11 @@ static int __init nbd_init(void)
return -EIO;
}
+ if (genl_register_family(&nbd_genl_family)) {
+ unregister_blkdev(NBD_MAJOR, "nbd");
+ destroy_workqueue(recv_workqueue);
+ return -EINVAL;
+ }
nbd_dbg_init();
mutex_lock(&nbd_index_mutex);
@@ -1369,6 +1610,7 @@ static void __exit nbd_cleanup(void)
idr_for_each(&nbd_index_idr, &nbd_exit_cb, NULL);
idr_destroy(&nbd_index_idr);
+ genl_unregister_family(&nbd_genl_family);
destroy_workqueue(recv_workqueue);
unregister_blkdev(NBD_MAJOR, "nbd");
}
diff --git a/include/uapi/linux/nbd-netlink.h b/include/uapi/linux/nbd-netlink.h
new file mode 100644
index 0000000..fd0f4e4
--- /dev/null
+++ b/include/uapi/linux/nbd-netlink.h
@@ -0,0 +1,69 @@
+/*
+ * Copyright (C) 2017 Facebook. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public
+ * License v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; if not, write to the
+ * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
+ * Boston, MA 021110-1307, USA.
+ */
+#ifndef _UAPILINUX_NBD_NETLINK_H
+#define _UAPILINUX_NBD_NETLINK_H
+
+#define NBD_GENL_FAMILY_NAME "nbd"
+#define NBD_GENL_VERSION 0x1
+
+/* Configuration policy attributes, used for CONNECT */
+enum {
+ NBD_ATTR_UNSPEC,
+ NBD_ATTR_INDEX,
+ NBD_ATTR_SIZE_BYTES,
+ NBD_ATTR_BLOCK_SIZE_BYTES,
+ NBD_ATTR_TIMEOUT,
+ NBD_ATTR_SERVER_FLAGS,
+ NBD_ATTR_CLIENT_FLAGS,
+ NBD_ATTR_SOCKETS,
+ __NBD_ATTR_MAX,
+};
+#define NBD_ATTR_MAX (__NBD_ATTR_MAX - 1)
+
+/*
+ * This is the format for multiple sockets with NBD_ATTR_SOCKETS
+ *
+ * [NBD_ATTR_SOCKETS]
+ * [NBD_SOCK_ITEM]
+ * [NBD_SOCK_FD]
+ * [NBD_SOCK_ITEM]
+ * [NBD_SOCK_FD]
+ */
+enum {
+ NBD_SOCK_ITEM_UNSPEC,
+ NBD_SOCK_ITEM,
+ __NBD_SOCK_ITEM_MAX,
+};
+#define NBD_SOCK_ITEM_MAX (__NBD_SOCK_ITEM_MAX - 1)
+
+enum {
+ NBD_SOCK_UNSPEC,
+ NBD_SOCK_FD,
+ __NBD_SOCK_MAX,
+};
+#define NBD_SOCK_MAX (__NBD_SOCK_MAX - 1)
+
+enum {
+ NBD_CMD_UNSPEC,
+ NBD_CMD_CONNECT,
+ NBD_CMD_DISCONNECT,
+ __NBD_CMD_MAX,
+};
+#define NBD_CMD_MAX (__NBD_CMD_MAX - 1)
+
+#endif /* _UAPILINUX_NBD_NETLINK_H */
--
2.7.4
^ permalink raw reply related
* Re: [PATCHv2 2/2] nvme: Complete all stuck requests
From: Keith Busch @ 2017-02-28 16:57 UTC (permalink / raw)
To: Artur Paszkiewicz
Cc: Jens Axboe, Sagi Grimberg, Marc MERLIN, linux-nvme, linux-block,
Christoph Hellwig
In-Reply-To: <913d4d24-b80b-b7f6-2374-8095bc6fc047@intel.com>
On Tue, Feb 28, 2017 at 08:42:19AM +0100, Artur Paszkiewicz wrote:
>
> I'm observing the same thing when hibernating during mdraid resync on
> nvme - it hangs in blk_mq_freeze_queue_wait() after "Disabling non-boot
> CPUs ...".
The patch guarantees forward progress for blk-mq's hot-cpu notifier on
nvme request queues by failing all entered requests. It sounds like some
part of your setup needs those requests to succeed in order to hibernate.
If your mdraid uses a stacking request_queue that submits retries while
it's request queue is entered, that may explain how you remain stuck
at blk_mq_freeze_queue_wait.
> This patch did not help but when I put nvme_wait_freeze()
> right after nvme_start_freeze() it appeared to be working. Maybe the
> difference here is that requests are submitted from a non-freezable
> kernel thread (md sync_thread)?
Wait freeze prior to quiescing the queue is ok when the controller is
functioning, but it'd be impossible to complete a reset if the controller
is in a failed or degraded state.
We probably want to give those requests a chance to succeed, and I think
we'd need to be able to timeout the freeze wait. Below are two patches
I tested. Prior to these, the fio test would report IO errors from some
of its jobs; no errors with these.
---
block/blk-mq.c | 9 +++++++++
include/linux/blk-mq.h | 2 ++
2 files changed, 11 insertions(+)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 8da2c04..a5e66a7 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -81,6 +81,15 @@ void blk_mq_freeze_queue_wait(struct request_queue *q)
}
EXPORT_SYMBOL_GPL(blk_mq_freeze_queue_wait);
+int blk_mq_freeze_queue_wait_timeout(struct request_queue *q,
+ unsigned long timeout)
+{
+ return wait_event_timeout(q->mq_freeze_wq,
+ percpu_ref_is_zero(&q->q_usage_counter),
+ timeout);
+}
+EXPORT_SYMBOL_GPL(blk_mq_freeze_queue_wait_timeout);
+
/*
* Guarantee no request is in use, so we can change any data structure of
* the queue afterward.
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 8dacf68..b296a90 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -246,6 +246,8 @@ void blk_mq_freeze_queue(struct request_queue *q);
void blk_mq_unfreeze_queue(struct request_queue *q);
void blk_mq_freeze_queue_start(struct request_queue *q);
void blk_mq_freeze_queue_wait(struct request_queue *q);
+int blk_mq_freeze_queue_wait_timeout(struct request_queue *q,
+ unsigned long timeout);
int blk_mq_reinit_tagset(struct blk_mq_tag_set *set);
int blk_mq_map_queues(struct blk_mq_tag_set *set);
--
---
drivers/nvme/host/core.c | 14 ++++++++++++++
drivers/nvme/host/nvme.h | 1 +
drivers/nvme/host/pci.c | 18 ++++++++++++++----
3 files changed, 29 insertions(+), 4 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9e99b94..9b3b57f 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2355,6 +2355,20 @@ void nvme_unfreeze(struct nvme_ctrl *ctrl)
}
EXPORT_SYMBOL_GPL(nvme_unfreeze);
+void nvme_wait_freeze_timeout(struct nvme_ctrl *ctrl, long timeout)
+{
+ struct nvme_ns *ns;
+
+ mutex_lock(&ctrl->namespaces_mutex);
+ list_for_each_entry(ns, &ctrl->namespaces, list) {
+ timeout = blk_mq_freeze_queue_wait_timeout(ns->queue, timeout);
+ if (timeout <= 0)
+ break;
+ }
+ mutex_unlock(&ctrl->namespaces_mutex);
+}
+EXPORT_SYMBOL_GPL(nvme_wait_freeze_timeout);
+
void nvme_wait_freeze(struct nvme_ctrl *ctrl)
{
struct nvme_ns *ns;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 62af901..2aa20e3 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -296,6 +296,7 @@ void nvme_start_queues(struct nvme_ctrl *ctrl);
void nvme_kill_queues(struct nvme_ctrl *ctrl);
void nvme_unfreeze(struct nvme_ctrl *ctrl);
void nvme_wait_freeze(struct nvme_ctrl *ctrl);
+void nvme_wait_freeze_timeout(struct nvme_ctrl *ctrl, long timeout);
void nvme_start_freeze(struct nvme_ctrl *ctrl);
#define NVME_QID_ANY -1
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index b266fb9..a7a423f 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1691,22 +1691,32 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
{
int i, queues;
u32 csts = -1;
+ bool dead;
+ struct pci_dev *pdev = to_pci_dev(dev->dev);
del_timer_sync(&dev->watchdog_timer);
mutex_lock(&dev->shutdown_lock);
if (shutdown)
nvme_start_freeze(&dev->ctrl);
- if (pci_is_enabled(to_pci_dev(dev->dev))) {
- nvme_stop_queues(&dev->ctrl);
+ if (pci_is_enabled(pdev))
csts = readl(dev->bar + NVME_REG_CSTS);
- }
+ dead = (csts & NVME_CSTS_CFS) || !(csts & NVME_CSTS_RDY) ||
+ pdev->error_state != pci_channel_io_normal;
+
+ /*
+ * Give the controller a chance to complete all entered requests if
+ * doing a safe shutdown.
+ */
+ if (!dead && shutdown)
+ nvme_wait_freeze_timeout(&dev->ctrl, NVME_IO_TIMEOUT);
+ nvme_stop_queues(&dev->ctrl);
queues = dev->online_queues - 1;
for (i = dev->queue_count - 1; i > 0; i--)
nvme_suspend_queue(dev->queues[i]);
- if (csts & NVME_CSTS_CFS || !(csts & NVME_CSTS_RDY)) {
+ if (dead) {
/* A device might become IO incapable very soon during
* probe, before the admin queue is configured. Thus,
* queue_count can be 0 here.
--
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related
* Re: [PATCH 2/3] block: Separate zone requests from medium access requests
From: Bart Van Assche @ 2017-02-28 16:57 UTC (permalink / raw)
To: linux-scsi@vger.kernel.org, linux-block@vger.kernel.org,
Damien Le Moal, martin.petersen@oracle.com, axboe@kernel.dk
Cc: hch@lst.de, hare@suse.de, MPT-FusionLinux.pdl@broadcom.com
In-Reply-To: <20170228102535.15966-3-damien.lemoal@wdc.com>
On Tue, 2017-02-28 at 19:25 +0900, Damien Le Moal wrote:
> From: Bart Van Assche <bart.vanassche@sandisk.com>
>=20
> Use blk_rq_accesses_medium() instead of !blk_rq_is_passthrough() to
> ensure that code that is intended for normal medium access requests,
> e.g. DISCARD, READ and WRITE requests, is not applied to
> REQ_OP_ZONE_REPORT requests nor to REQ_OP_ZONE_RESET requests.
> This allows excluding these zone requests from request accounting
> and from request scheduling.
>=20
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Hello Damien,
Since you posted this patch you should have added your Signed-off-by. And
since you have edited this patch, you should have documented what you have
changed.
> static inline void req_set_nomerge(struct request_queue *q, struct reque=
st *req)
> diff --git a/block/elevator.c b/block/elevator.c
> index 699d10f..cbf81c6 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -635,16 +635,20 @@ void __elv_add_request(struct request_queue *q, str=
uct request *rq, int where)
> =20
> rq->q =3D q;
> =20
> - if (rq->rq_flags & RQF_SOFTBARRIER) {
> + if (!blk_rq_accesses_medium(rq)) {
> + /* Do not schedule zone requests */
> + where =3D ELEVATOR_INSERT_FRONT;
> + } if (rq->rq_flags & RQF_SOFTBARRIER) {
This change was not in the patch I sent to you. Additionally, this change
doesn't look properly formatted. Please make sure that the second "if" star=
ts
on a new line.
> }
> } else if (!(rq->rq_flags & RQF_ELVPRIV) &&
> (where =3D=3D ELEVATOR_INSERT_SORT ||
> - where =3D=3D ELEVATOR_INSERT_SORT_MERGE))
> + where =3D=3D ELEVATOR_INSERT_SORT_MERGE)) {
> where =3D ELEVATOR_INSERT_BACK;
> + }
This change wasn't in my patch either. Since this change only adds a pair o=
f
braces, can it be left out?
Thanks,
Bart.=
^ permalink raw reply
* Re: [PATCH 11/16] mmc: block: shuffle retry and error handling
From: Bartlomiej Zolnierkiewicz @ 2017-02-28 17:45 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-mmc, Ulf Hansson, Adrian Hunter, Paolo Valente,
Chunyan Zhang, Baolin Wang, linux-block, Jens Axboe,
Christoph Hellwig, Arnd Bergmann
In-Reply-To: <20170209153403.9730-12-linus.walleij@linaro.org>
On Thursday, February 09, 2017 04:33:58 PM Linus Walleij wrote:
> Instead of doing retries at the same time as trying to submit new
> requests, do the retries when the request is reported as completed
> by the driver, in the finalization worker.
>
> This is achieved by letting the core worker call back into the block
> layer using mmc_blk_rw_done(), that will read the status and repeatedly
> try to hammer the request using single request etc by calling back to
> the core layer using mmc_restart_areq()
>
> The beauty of it is that the completion will not complete until the
> block layer has had the opportunity to hammer a bit at the card using
> a bunch of different approaches in the while() loop in
> mmc_blk_rw_done()
>
> The algorithm for recapture, retry and handle errors is essentially
> identical to the one we used to have in mmc_blk_issue_rw_rq(),
> only augmented to get called in another path.
>
> We have to add and initialize a pointer back to the struct mmc_queue
> from the struct mmc_queue_req to find the queue from the asynchronous
> request.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
It seems that after this change we can end up queuing more
work for kthread from the kthread worker itself and wait
inside it for this nested work to complete. I hope that
you've tested it with simulating errors and it all works.
Under this assumption:
Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Also some very minor nit:
+ case MMC_BLK_DATA_ERR: {
+ int err;
+ err = mmc_blk_reset(md, host, type);
During the code movement CodingStyle suffered.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
* Re: [PATCH 12/16] mmc: queue: stop flushing the pipeline with NULL
From: Bartlomiej Zolnierkiewicz @ 2017-02-28 18:03 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-mmc, Ulf Hansson, Adrian Hunter, Paolo Valente,
Chunyan Zhang, Baolin Wang, linux-block, Jens Axboe,
Christoph Hellwig, Arnd Bergmann
In-Reply-To: <20170209153403.9730-13-linus.walleij@linaro.org>
On Thursday, February 09, 2017 04:33:59 PM Linus Walleij wrote:
> Remove all the pipeline flush: i.e. repeatedly sending NULL
> down to the core layer to flush out asynchronous requests,
> and also sending NULL after "special" commands to achieve the
> same flush.
>
> Instead: let the "special" commands wait for any ongoing
> asynchronous transfers using the completion, and apart from
> that expect the core.c and block.c layers to deal with the
> ongoing requests autonomously without any "push" from the
> queue.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> drivers/mmc/core/block.c | 80 +++++++++++++++++-------------------------------
> drivers/mmc/core/core.c | 37 ++++++++++------------
> drivers/mmc/core/queue.c | 18 ++++++++---
> include/linux/mmc/core.h | 5 ++-
> 4 files changed, 60 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 0bd9070f5f2e..4952a105780e 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -1753,42 +1753,27 @@ void mmc_blk_rw_done(struct mmc_async_req *areq,
>
> static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *new_req)
> {
> - enum mmc_blk_status status;
> - struct mmc_async_req *new_areq;
> - struct mmc_async_req *old_areq;
> struct mmc_card *card = mq->card;
>
> - if (!new_req && !mq->mqrq_prev->req)
> + if (!new_req) {
> + pr_err("%s: NULL request!\n", __func__);
> return;
> + }
>
> - if (new_req) {
> - /*
> - * When 4KB native sector is enabled, only 8 blocks
> - * multiple read or write is allowed
> - */
> - if (mmc_large_sector(card) &&
> - !IS_ALIGNED(blk_rq_sectors(new_req), 8)) {
> - pr_err("%s: Transfer size is not 4KB sector size aligned\n",
> - new_req->rq_disk->disk_name);
> - mmc_blk_rw_cmd_abort(card, new_req);
> - return;
> - }
> -
> - mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
> - new_areq = &mq->mqrq_cur->areq;
> - } else
> - new_areq = NULL;
> -
> - old_areq = mmc_start_areq(card->host, new_areq, &status);
> - if (!old_areq) {
> - /*
> - * We have just put the first request into the pipeline
> - * and there is nothing more to do until it is
> - * complete.
> - */
> + /*
> + * When 4KB native sector is enabled, only 8 blocks
> + * multiple read or write is allowed
> + */
> + if (mmc_large_sector(card) &&
> + !IS_ALIGNED(blk_rq_sectors(new_req), 8)) {
> + pr_err("%s: Transfer size is not 4KB sector size aligned\n",
> + new_req->rq_disk->disk_name);
> + mmc_blk_rw_cmd_abort(card, new_req);
> return;
> }
> - /* FIXME: yes, we just disregard the old_areq */
> +
> + mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
> + mmc_start_areq(card->host, &mq->mqrq_cur->areq);
> }
>
> void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
> @@ -1796,48 +1781,39 @@ void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
> int ret;
> struct mmc_blk_data *md = mq->blkdata;
> struct mmc_card *card = md->queue.card;
> - bool req_is_special = mmc_req_is_special(req);
> -
> - if (req && !mq->mqrq_prev->req)
> - /* claim host only for the first request */
> - mmc_get_card(card);
>
> ret = mmc_blk_part_switch(card, md);
> if (ret) {
> if (req) {
> blk_end_request_all(req, -EIO);
> }
> - goto out;
> + return;
> }
>
> if (req && req_op(req) == REQ_OP_DISCARD) {
> /* complete ongoing async transfer before issuing discard */
> - if (card->host->areq)
> - mmc_blk_issue_rw_rq(mq, NULL);
> + if (card->host->areq) {
> + wait_for_completion(&card->host->areq->complete);
> + card->host->areq = NULL;
> + }
> mmc_blk_issue_discard_rq(mq, req);
> } else if (req && req_op(req) == REQ_OP_SECURE_ERASE) {
> /* complete ongoing async transfer before issuing secure erase*/
> - if (card->host->areq)
> - mmc_blk_issue_rw_rq(mq, NULL);
> + if (card->host->areq) {
> + wait_for_completion(&card->host->areq->complete);
> + card->host->areq = NULL;
> + }
> mmc_blk_issue_secdiscard_rq(mq, req);
> } else if (req && req_op(req) == REQ_OP_FLUSH) {
> /* complete ongoing async transfer before issuing flush */
> - if (card->host->areq)
> - mmc_blk_issue_rw_rq(mq, NULL);
> + if (card->host->areq) {
> + wait_for_completion(&card->host->areq->complete);
> + card->host->areq = NULL;
> + }
> mmc_blk_issue_flush(mq, req);
> } else {
> mmc_blk_issue_rw_rq(mq, req);
> }
> -
> -out:
> - if (!req || req_is_special)
> - /*
> - * Release host when there are no more requests
> - * and after special request(discard, flush) is done.
> - * In case sepecial request, there is no reentry to
> - * the 'mmc_blk_issue_rq' with 'mqrq_prev->req'.
> - */
> - mmc_put_card(card);
> }
>
> static inline int mmc_blk_readonly(struct mmc_card *card)
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 34337ef6705e..03c290e5e2c9 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -667,42 +667,37 @@ EXPORT_SYMBOL(mmc_restart_areq);
> * return the completed request. If there is no ongoing request, NULL
> * is returned without waiting. NULL is not an error condition.
> */
> -struct mmc_async_req *mmc_start_areq(struct mmc_host *host,
> - struct mmc_async_req *areq,
> - enum mmc_blk_status *ret_stat)
> +int mmc_start_areq(struct mmc_host *host,
> + struct mmc_async_req *areq)
> {
> - enum mmc_blk_status status;
> - int start_err = 0;
> + int ret;
> struct mmc_async_req *previous = host->areq;
>
> /* Prepare a new request */
> - if (areq)
> - mmc_pre_req(host, areq->mrq);
> + if (!areq) {
> + pr_err("%s: NULL asynchronous request!\n", __func__);
> + return -EIO;
> + }
> +
> + mmc_pre_req(host, areq->mrq);
>
> /* Finalize previous request, if there is one */
> if (previous)
> wait_for_completion(&previous->complete);
>
> - status = MMC_BLK_SUCCESS;
> - if (ret_stat)
> - *ret_stat = status;
> -
> /* Fine so far, start the new request! */
> - if (status == MMC_BLK_SUCCESS && areq) {
> - init_completion(&areq->complete);
> - start_err = __mmc_start_data_req(host, areq->mrq);
> - }
> + init_completion(&areq->complete);
> + ret = __mmc_start_data_req(host, areq->mrq);
>
> /* Cancel a prepared request if it was not started. */
> - if ((status != MMC_BLK_SUCCESS || start_err) && areq)
> + if (ret) {
> mmc_post_req(host, areq->mrq, -EINVAL);
> -
> - if (status != MMC_BLK_SUCCESS)
> host->areq = NULL;
> - else
> - host->areq = areq;
> + pr_err("%s: failed to start request\n", __func__);
> + }
> + host->areq = areq;
>
> - return previous;
> + return ret;
> }
> EXPORT_SYMBOL(mmc_start_areq);
>
> diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
> index ae6837317fe0..c9f28de7b0f4 100644
> --- a/drivers/mmc/core/queue.c
> +++ b/drivers/mmc/core/queue.c
> @@ -53,6 +53,7 @@ static int mmc_queue_thread(void *d)
> {
> struct mmc_queue *mq = d;
> struct request_queue *q = mq->queue;
> + bool claimed_host = false;
>
> current->flags |= PF_MEMALLOC;
>
> @@ -67,9 +68,11 @@ static int mmc_queue_thread(void *d)
> mq->mqrq_cur->req = req;
> spin_unlock_irq(q->queue_lock);
>
> - if (req || mq->mqrq_prev->req) {
> + if (req) {
> bool req_is_special = mmc_req_is_special(req);
>
> + if (!claimed_host)
> + mmc_get_card(mq->card);
missing
claimed_host = true;
?
> set_current_state(TASK_RUNNING);
> mmc_blk_issue_rq(mq, req);
> cond_resched();
> @@ -78,11 +81,14 @@ static int mmc_queue_thread(void *d)
> * and vice versa.
> * In case of special requests, current request
> * has been finished. Do not assign it to previous
> - * request.
> + * request. Always unclaim the host after special
> + * commands.
> */
> - if (req_is_special)
> + if (req_is_special) {
> mq->mqrq_cur->req = NULL;
> -
> + mmc_put_card(mq->card);
> + claimed_host = false;
> + }
> mq->mqrq_prev->brq.mrq.data = NULL;
> mq->mqrq_prev->req = NULL;
> swap(mq->mqrq_prev, mq->mqrq_cur);
> @@ -97,6 +103,10 @@ static int mmc_queue_thread(void *d)
> down(&mq->thread_sem);
> }
> } while (1);
> +
> + if (claimed_host)
claimed_host is never set to true
> + mmc_put_card(mq->card);
> +
> up(&mq->thread_sem);
>
> return 0;
> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> index 55b45dcddee6..af651e723ba2 100644
> --- a/include/linux/mmc/core.h
> +++ b/include/linux/mmc/core.h
> @@ -160,9 +160,8 @@ struct mmc_async_req;
>
> void mmc_finalize_areq(struct kthread_work *work);
> int mmc_restart_areq(struct mmc_host *host, struct mmc_async_req *areq);
> -struct mmc_async_req *mmc_start_areq(struct mmc_host *host,
> - struct mmc_async_req *areq,
> - enum mmc_blk_status *ret_stat);
> +int mmc_start_areq(struct mmc_host *host,
> + struct mmc_async_req *areq);
> void mmc_wait_for_req(struct mmc_host *host, struct mmc_request *mrq);
> int mmc_wait_for_cmd(struct mmc_host *host, struct mmc_command *cmd,
> int retries);
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
* Re: [PATCH 13/16] mmc: queue: issue struct mmc_queue_req items
From: Bartlomiej Zolnierkiewicz @ 2017-02-28 18:10 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-mmc, Ulf Hansson, Adrian Hunter, Paolo Valente,
Chunyan Zhang, Baolin Wang, linux-block, Jens Axboe,
Christoph Hellwig, Arnd Bergmann
In-Reply-To: <20170209153403.9730-14-linus.walleij@linaro.org>
On Thursday, February 09, 2017 04:34:00 PM Linus Walleij wrote:
> Instead of passing two pointers around and messing and reassigning
> to the left and right, issue mmc_queue_req and dereference
> the queue from the request where needed. The struct mmc_queue_req
> is the thing that has a lifecycle after all: this is what we are
> keepin in out queue. Augment all users to be passed the struct
> mmc_queue_req as well.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> drivers/mmc/core/block.c | 88 ++++++++++++++++++++++++------------------------
> drivers/mmc/core/block.h | 5 ++-
> drivers/mmc/core/queue.c | 6 ++--
> 3 files changed, 50 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 4952a105780e..628a22b9bf41 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -1151,9 +1151,9 @@ int mmc_access_rpmb(struct mmc_queue *mq)
> return false;
> }
>
> -static void mmc_blk_issue_discard_rq(struct mmc_queue *mq, struct request *req)
> +static void mmc_blk_issue_discard_rq(struct mmc_queue_req *mq_rq)
> {
> - struct mmc_blk_data *md = mq->blkdata;
> + struct mmc_blk_data *md = mq_rq->mq->blkdata;
> struct mmc_card *card = md->queue.card;
> unsigned int from, nr, arg;
> int err = 0, type = MMC_BLK_DISCARD;
> @@ -1163,8 +1163,8 @@ static void mmc_blk_issue_discard_rq(struct mmc_queue *mq, struct request *req)
> goto fail;
> }
>
> - from = blk_rq_pos(req);
> - nr = blk_rq_sectors(req);
> + from = blk_rq_pos(mq_rq->req);
> + nr = blk_rq_sectors(mq_rq->req);
>
> if (mmc_can_discard(card))
> arg = MMC_DISCARD_ARG;
> @@ -1188,13 +1188,12 @@ static void mmc_blk_issue_discard_rq(struct mmc_queue *mq, struct request *req)
> if (!err)
> mmc_blk_reset_success(md, type);
> fail:
> - blk_end_request(req, err, blk_rq_bytes(req));
> + blk_end_request(mq_rq->req, err, blk_rq_bytes(mq_rq->req));
> }
>
> -static void mmc_blk_issue_secdiscard_rq(struct mmc_queue *mq,
> - struct request *req)
> +static void mmc_blk_issue_secdiscard_rq(struct mmc_queue_req *mq_rq)
> {
> - struct mmc_blk_data *md = mq->blkdata;
> + struct mmc_blk_data *md = mq_rq->mq->blkdata;
> struct mmc_card *card = md->queue.card;
> unsigned int from, nr, arg;
> int err = 0, type = MMC_BLK_SECDISCARD;
> @@ -1204,8 +1203,8 @@ static void mmc_blk_issue_secdiscard_rq(struct mmc_queue *mq,
> goto out;
> }
>
> - from = blk_rq_pos(req);
> - nr = blk_rq_sectors(req);
> + from = blk_rq_pos(mq_rq->req);
> + nr = blk_rq_sectors(mq_rq->req);
>
> if (mmc_can_trim(card) && !mmc_erase_group_aligned(card, from, nr))
> arg = MMC_SECURE_TRIM1_ARG;
> @@ -1253,12 +1252,12 @@ static void mmc_blk_issue_secdiscard_rq(struct mmc_queue *mq,
> if (!err)
> mmc_blk_reset_success(md, type);
> out:
> - blk_end_request(req, err, blk_rq_bytes(req));
> + blk_end_request(mq_rq->req, err, blk_rq_bytes(mq_rq->req));
> }
>
> -static void mmc_blk_issue_flush(struct mmc_queue *mq, struct request *req)
> +static void mmc_blk_issue_flush(struct mmc_queue_req *mq_rq)
> {
> - struct mmc_blk_data *md = mq->blkdata;
> + struct mmc_blk_data *md = mq_rq->mq->blkdata;
> struct mmc_card *card = md->queue.card;
> int ret = 0;
>
> @@ -1266,7 +1265,7 @@ static void mmc_blk_issue_flush(struct mmc_queue *mq, struct request *req)
> if (ret)
> ret = -EIO;
>
> - blk_end_request_all(req, ret);
> + blk_end_request_all(mq_rq->req, ret);
> }
>
> /*
> @@ -1614,11 +1613,13 @@ static void mmc_blk_rw_cmd_abort(struct mmc_card *card, struct request *req)
> * @mq: the queue with the card and host to restart
> * @req: a new request that want to be started after the current one
> */
> -static void mmc_blk_rw_try_restart(struct mmc_queue *mq)
> +static void mmc_blk_rw_try_restart(struct mmc_queue_req *mq_rq)
> {
> + struct mmc_queue *mq = mq_rq->mq;
> +
> /* Proceed and try to restart the current async request */
> - mmc_blk_rw_rq_prep(mq->mqrq_cur, mq->card, 0, mq);
> - mmc_restart_areq(mq->card->host, &mq->mqrq_cur->areq);
> + mmc_blk_rw_rq_prep(mq_rq, mq->card, 0, mq);
> + mmc_restart_areq(mq->card->host, &mq_rq->areq);
> }
>
> void mmc_blk_rw_done(struct mmc_async_req *areq,
> @@ -1676,11 +1677,11 @@ void mmc_blk_rw_done(struct mmc_async_req *areq,
> req_pending = mmc_blk_rw_cmd_err(md, card, brq, old_req, req_pending);
> if (mmc_blk_reset(md, host, type)) {
> mmc_blk_rw_cmd_abort(card, old_req);
> - mmc_blk_rw_try_restart(mq);
> + mmc_blk_rw_try_restart(mq_rq);
> return;
> }
> if (!req_pending) {
> - mmc_blk_rw_try_restart(mq);
> + mmc_blk_rw_try_restart(mq_rq);
> return;
> }
> break;
> @@ -1693,7 +1694,7 @@ void mmc_blk_rw_done(struct mmc_async_req *areq,
> if (!mmc_blk_reset(md, host, type))
> break;
> mmc_blk_rw_cmd_abort(card, old_req);
> - mmc_blk_rw_try_restart(mq);
> + mmc_blk_rw_try_restart(mq_rq);
> return;
> case MMC_BLK_DATA_ERR: {
> int err;
> @@ -1702,7 +1703,7 @@ void mmc_blk_rw_done(struct mmc_async_req *areq,
> break;
> if (err == -ENODEV) {
> mmc_blk_rw_cmd_abort(card, old_req);
> - mmc_blk_rw_try_restart(mq);
> + mmc_blk_rw_try_restart(mq_rq);
> return;
> }
> /* Fall through */
> @@ -1723,19 +1724,19 @@ void mmc_blk_rw_done(struct mmc_async_req *areq,
> req_pending = blk_end_request(old_req, -EIO,
> brq->data.blksz);
> if (!req_pending) {
> - mmc_blk_rw_try_restart(mq);
> + mmc_blk_rw_try_restart(mq_rq);
> return;
> }
> break;
> case MMC_BLK_NOMEDIUM:
> mmc_blk_rw_cmd_abort(card, old_req);
> - mmc_blk_rw_try_restart(mq);
> + mmc_blk_rw_try_restart(mq_rq);
> return;
> default:
> pr_err("%s: Unhandled return value (%d)",
> old_req->rq_disk->disk_name, status);
> mmc_blk_rw_cmd_abort(card, old_req);
> - mmc_blk_rw_try_restart(mq);
> + mmc_blk_rw_try_restart(mq_rq);
> return;
> }
>
> @@ -1747,15 +1748,16 @@ void mmc_blk_rw_done(struct mmc_async_req *areq,
> mmc_blk_rw_rq_prep(mq_rq, card,
> disable_multi, mq);
> mq_rq->brq.retune_retry_done = retune_retry_done;
> - mmc_restart_areq(host, &mq->mqrq_cur->areq);
> + mmc_restart_areq(host, &mq_rq->areq);
> }
> }
>
> -static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *new_req)
> +static void mmc_blk_issue_rw_rq(struct mmc_queue_req *mq_rq)
> {
> + struct mmc_queue *mq = mq_rq->mq;
> struct mmc_card *card = mq->card;
>
> - if (!new_req) {
> + if (!mq_rq->req) {
> pr_err("%s: NULL request!\n", __func__);
> return;
> }
> @@ -1765,54 +1767,52 @@ static void mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *new_req)
> * multiple read or write is allowed
> */
> if (mmc_large_sector(card) &&
> - !IS_ALIGNED(blk_rq_sectors(new_req), 8)) {
> + !IS_ALIGNED(blk_rq_sectors(mq_rq->req), 8)) {
> pr_err("%s: Transfer size is not 4KB sector size aligned\n",
> - new_req->rq_disk->disk_name);
> - mmc_blk_rw_cmd_abort(card, new_req);
> + mq_rq->req->rq_disk->disk_name);
> + mmc_blk_rw_cmd_abort(card, mq_rq->req);
> return;
> }
>
> - mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
> - mmc_start_areq(card->host, &mq->mqrq_cur->areq);
> + mmc_blk_rw_rq_prep(mq_rq, card, 0, mq);
> + mmc_start_areq(card->host, &mq_rq->areq);
> }
>
> -void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
> +void mmc_blk_issue_rq(struct mmc_queue_req *mq_rq)
> {
> int ret;
> - struct mmc_blk_data *md = mq->blkdata;
> + struct mmc_blk_data *md = mq_rq->mq->blkdata;
> struct mmc_card *card = md->queue.card;
>
> ret = mmc_blk_part_switch(card, md);
> if (ret) {
> - if (req) {
dropping of req checking should belong to some earlier patch
> - blk_end_request_all(req, -EIO);
> - }
> + blk_end_request_all(mq_rq->req, -EIO);
> return;
> }
>
> - if (req && req_op(req) == REQ_OP_DISCARD) {
> + if (req_op(mq_rq->req) == REQ_OP_DISCARD) {
ditto
> /* complete ongoing async transfer before issuing discard */
> if (card->host->areq) {
> wait_for_completion(&card->host->areq->complete);
> card->host->areq = NULL;
> }
> - mmc_blk_issue_discard_rq(mq, req);
> - } else if (req && req_op(req) == REQ_OP_SECURE_ERASE) {
ditto
> + mmc_blk_issue_discard_rq(mq_rq);
> + } else if (req_op(mq_rq->req) == REQ_OP_SECURE_ERASE) {
> /* complete ongoing async transfer before issuing secure erase*/
> if (card->host->areq) {
> wait_for_completion(&card->host->areq->complete);
> card->host->areq = NULL;
> }
> - mmc_blk_issue_secdiscard_rq(mq, req);
> - } else if (req && req_op(req) == REQ_OP_FLUSH) {
> + mmc_blk_issue_secdiscard_rq(mq_rq);
> + } else if (req_op(mq_rq->req) == REQ_OP_FLUSH) {
ditto
> /* complete ongoing async transfer before issuing flush */
> if (card->host->areq) {
> wait_for_completion(&card->host->areq->complete);
> card->host->areq = NULL;
> }
> - mmc_blk_issue_flush(mq, req);
> + mmc_blk_issue_flush(mq_rq);
> } else {
> - mmc_blk_issue_rw_rq(mq, req);
> + mmc_blk_issue_rw_rq(mq_rq);
> }
> }
>
> diff --git a/drivers/mmc/core/block.h b/drivers/mmc/core/block.h
> index b4b489911599..0326fa5d8217 100644
> --- a/drivers/mmc/core/block.h
> +++ b/drivers/mmc/core/block.h
> @@ -3,10 +3,9 @@
>
> struct mmc_async_req;
> enum mmc_blk_status;
> -struct mmc_queue;
> -struct request;
> +struct mmc_queue_req;
>
> void mmc_blk_rw_done(struct mmc_async_req *areq, enum mmc_blk_status status);
> -void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req);
> +void mmc_blk_issue_rq(struct mmc_queue_req *mq_rq);
>
> #endif
> diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
> index c9f28de7b0f4..c4e1ced55796 100644
> --- a/drivers/mmc/core/queue.c
> +++ b/drivers/mmc/core/queue.c
> @@ -54,6 +54,7 @@ static int mmc_queue_thread(void *d)
> struct mmc_queue *mq = d;
> struct request_queue *q = mq->queue;
> bool claimed_host = false;
> + struct mmc_queue_req *mq_rq;
>
> current->flags |= PF_MEMALLOC;
>
> @@ -65,7 +66,8 @@ static int mmc_queue_thread(void *d)
> set_current_state(TASK_INTERRUPTIBLE);
> req = blk_fetch_request(q);
> mq->asleep = false;
> - mq->mqrq_cur->req = req;
> + mq_rq = mq->mqrq_cur;
> + mq_rq->req = req;
> spin_unlock_irq(q->queue_lock);
>
> if (req) {
> @@ -74,7 +76,7 @@ static int mmc_queue_thread(void *d)
> if (!claimed_host)
> mmc_get_card(mq->card);
> set_current_state(TASK_RUNNING);
> - mmc_blk_issue_rq(mq, req);
> + mmc_blk_issue_rq(mq_rq);
> cond_resched();
> /*
> * Current request becomes previous request
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
* Re: [PATCH 14/16] mmc: queue: get/put struct mmc_queue_req
From: Bartlomiej Zolnierkiewicz @ 2017-02-28 18:21 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-mmc, Ulf Hansson, Adrian Hunter, Paolo Valente,
Chunyan Zhang, Baolin Wang, linux-block, Jens Axboe,
Christoph Hellwig, Arnd Bergmann
In-Reply-To: <20170209153403.9730-15-linus.walleij@linaro.org>
On Thursday, February 09, 2017 04:34:01 PM Linus Walleij wrote:
> The per-hardware-transaction struct mmc_queue_req is assigned
> from a pool of 2 requests using a current/previous scheme and
> then swapped around.
>
> This is confusing, especially if we need more than two to make
> our work efficient and parallel.
>
> Rewrite the mechanism to have a pool of struct mmc_queue_req
> and take one when we need one and put it back when we don't
> need it anymore.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
* Re: [PATCH] sbitmap: boundary checks for resized bitmap
From: Omar Sandoval @ 2017-02-28 19:15 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: Jens Axboe, Omar Sandoval, linux-block, Hannes Reinecke
In-Reply-To: <1487157042-5621-1-git-send-email-hare@suse.de>
On Wed, Feb 15, 2017 at 12:10:42PM +0100, Hannes Reinecke wrote:
> If the sbitmap gets resized we need to ensure not to overflow
> the original allocation. And we should limit the search in
> sbitmap_any_bit_set() to the available depth to avoid looking
> into unused space.
Hey, Hannes, I don't really like this change. It's easy enough for the
caller to keep track of this and check themselves if they really care. I
even included a comment in sbitmap.h to that effect:
/**
* sbitmap_resize() - Resize a &struct sbitmap.
* @sb: Bitmap to resize.
* @depth: New number of bits to resize to.
*
* Doesn't reallocate anything. It's up to the caller to ensure that the new
* depth doesn't exceed the depth that the sb was initialized with.
*/
As for the sbitmap_any_bit_set() change, the bits beyond the actual
depth should all be zero, so I don't think that change is worth it,
either.
Thanks!
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox