Re: [PATCH 4/4] Fix race between starved list processing and device removal
Hello, On Wed, Oct 10, 2012 at 05:10:35PM +0200, Bart Van Assche wrote: > Avoid that the sdev reference count can drop to zero before > the queue is run by scsi_run_queue(). Also avoid that the sdev > reference count can drop to zero in the same function by invoking > __blk_run_queue(). I think this is correct but again had to scratch my head quite a bit. It would be nice to start with brief explanation of starved_list, especially the way it's processed per-host instead of per-device. And then explain how it's broken(how nothing guarantees a device to be alive while on starved_list) and the fix. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] Make blk_cleanup_queue() wait until request_fn finished
Hello,
On Wed, Oct 10, 2012 at 05:09:04PM +0200, Bart Van Assche wrote:
> Some request_fn implementations, e.g. scsi_request_fn(), unlock
> the queue lock. Make sure that blk_cleanup_queue() waits until all
> active request_fn invocations have finished. This fixes a potential
> use-after-free at the end of scsi_request_fn(). Also, change the
> type of the 'drain' variable from bool to int to avoid that the
> highest bits of the request counters get ignored.
Similar comment. It would be great if you better separate what's
broken and how it's fixed. Kinda difficult to digest.
> @@ -308,7 +308,9 @@ void __blk_run_queue_uncond(struct request_queue *q)
> if (unlikely(blk_queue_dead(q)))
> return;
>
> + q->driver_active++;
> q->request_fn(q);
> + q->driver_active--;
Maybe q->request_fn_active is a better name?
> -void blk_drain_queue(struct request_queue *q, bool drain_all)
> +static void blk_drain_queue(struct request_queue *q, bool drain_all)
> {
> int i;
>
> while (true) {
> - bool drain = false;
> + int drain = 0;
I don't think this is necessary. bool conversion works fine
regardless how high the bits are. Isn't avoiding signed/unsigned
autocast maze one of the reasons why we're using bool to begin with?
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index f29a1a9..0e15374 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1517,10 +1517,6 @@ static void scsi_request_fn(struct request_queue *q)
> struct scsi_cmnd *cmd;
> struct request *req;
>
> - if(!get_device(&sdev->sdev_gendev))
> - /* We must be tearing the block queue down already */
> - return;
> -
> /*
>* To start with, we keep looping until the queue is empty, or until
>* the host is no longer able to accept any more requests.
> @@ -1629,11 +1625,7 @@ out_delay:
> if (sdev->device_busy == 0)
> blk_delay_queue(q, SCSI_QUEUE_DELAY);
> out:
> - /* must be careful here...if we trigger the ->remove() function
> - * we cannot be holding the q lock */
> - spin_unlock_irq(q->queue_lock);
> - put_device(&sdev->sdev_gendev);
> - spin_lock_irq(q->queue_lock);
> + ;
I think moving this out to a separate patch would be better.
Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] block: Avoid that request_fn is invoked on a dead queue
Hello,
On Wed, Oct 10, 2012 at 05:08:01PM +0200, Bart Van Assche wrote:
> A block driver may start cleaning up resources needed by its
> request_fn as soon as blk_cleanup_queue() finished, so request_fn
> must not be invoked after draining finished.
Can you please make the commit message more verbose preferably with
example crash trace? It's difficult to tell what bug it's trying to
fix how.
> /**
> + * __blk_run_queue_uncond - run a queue whether or not it has been stopped
> + * @q: The queue to run
> + *
> + * Description:
> + *Invoke request handling on a queue if there are any pending requests.
> + *May be used to restart request handling after a request has completed.
> + *This variant runs the queue whether or not the queue has been
> + *stopped. Must be called with the queue lock held and interrupts
> + *disabled. See also @blk_run_queue.
> + */
> +void __blk_run_queue_uncond(struct request_queue *q)
> +{
> + if (unlikely(blk_queue_dead(q)))
> + return;
> +
> + q->request_fn(q);
> +}
> +
> +/**
> * __blk_run_queue - run a single device queue
> * @q: The queue to run
> *
> @@ -305,7 +324,7 @@ void __blk_run_queue(struct request_queue *q)
> if (unlikely(blk_queue_stopped(q)))
> return;
>
> - q->request_fn(q);
> + __blk_run_queue_uncond(q);
__blk_run_queue_uncond() is a cold path and I don't think adding a
test there matters but I think it would be better if we avoid an extra
branch if possible for __blk_run_queue(). Can't we merge
blk_queue_stopped/dead() testing?
> @@ -401,6 +420,9 @@ void blk_drain_queue(struct request_queue *q, bool
> drain_all)
> }
> }
>
> + if (!drain && blk_queue_dying(q))
> + queue_flag_set(QUEUE_FLAG_DEAD, q);
> +
Wouldn't doing this in blk_cleanup_queue() be better? It may involve
an extra queue locking but I don't think that matter at all in that
path and doing things where they belong is far more important.
Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] block: Rename queue dead flag
On Wed, Oct 10, 2012 at 05:07:08PM +0200, Bart Van Assche wrote: > QUEUE_FLAG_DEAD is used to indicate that queuing new requests must > stop. After this flag has been set queue draining starts. However, > during the queue draining phase it is still safe to invoke the > queue's request_fn, so QUEUE_FLAG_DYING is a better name for this > flag. > > This patch is the result of running the command below and manually > fixing up indentation: > > git grep -lE 'blk_queue_dead|QUEUE_FLAG_DEAD' | > xargs sed -i.tmp -e 's/blk_queue_dead/blk_queue_dying/g' \ > -e 's/QUEUE_FLAG_DEAD/QUEUE_FLAG_DYING/g' > > Cc: James Bottomley > Cc: Mike Christie > Cc: Jens Axboe > Cc: Tejun Heo > Cc: Chanho Min > Signed-off-by: Bart Van Assche Acked-by: Tejun Heo Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] aacraid: SCSI dma mapping failure case handling
On 10/16/2012 10:59 PM, Mahesh Rajashekhara wrote:
> This patch handles SCSI dma mapping failure case. Reporting error code to the
> upper layer instead of BUG_ON().
>
> This patch is created against current upstream kernel.
>
> Signed-off-by: Mahesh Rajashekhara
> ---
> drivers/scsi/aacraid/aachba.c | 63
> +++-
> drivers/scsi/aacraid/aacraid.h |2 +-
> 2 files changed, 50 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
> index d79457a..efa2900 100644
> --- a/drivers/scsi/aacraid/aachba.c
> +++ b/drivers/scsi/aacraid/aachba.c
> @@ -971,6 +971,7 @@ static int aac_read_raw_io(struct fib * fib, struct
> scsi_cmnd * cmd, u64 lba, u3
> {
> struct aac_dev *dev = fib->dev;
> u16 fibsize, command;
> + unsigned long ret;
>
> aac_fib_init(fib);
> if (dev->comm_interface == AAC_COMM_MESSAGE_TYPE2 && !dev->sync_mode) {
> @@ -982,7 +983,10 @@ static int aac_read_raw_io(struct fib * fib, struct
> scsi_cmnd * cmd, u64 lba, u3
> readcmd2->byteCount = cpu_to_le32(count<<9);
> readcmd2->cid = cpu_to_le16(scmd_id(cmd));
> readcmd2->flags = cpu_to_le16(RIO2_IO_TYPE_READ);
> - aac_build_sgraw2(cmd, readcmd2,
> dev->scsi_host_ptr->sg_tablesize);
> + ret = aac_build_sgraw2(cmd, readcmd2,
> + dev->scsi_host_ptr->sg_tablesize);
> + if (ret < 0)
> + return ret;
Hi Mahesh, 'ret' is 'unsigned', the above test will not work.
Tomas
> command = ContainerRawIo2;
> fibsize = sizeof(struct aac_raw_io2) +
> ((le32_to_cpu(readcmd2->sgeCnt)-1) * sizeof(struct
> sge_ieee1212));
> @@ -996,7 +1000,9 @@ static int aac_read_raw_io(struct fib * fib, struct
> scsi_cmnd * cmd, u64 lba, u3
> readcmd->flags = cpu_to_le16(RIO_TYPE_READ);
> readcmd->bpTotal = 0;
> readcmd->bpComplete = 0;
> - aac_build_sgraw(cmd, &readcmd->sg);
> + ret = aac_build_sgraw(cmd, &readcmd->sg);
> + if (ret < 0)
> + return ret;
> command = ContainerRawIo;
> fibsize = sizeof(struct aac_raw_io) +
> ((le32_to_cpu(readcmd->sg.count)-1) * sizeof(struct
> sgentryraw));
> @@ -1019,6 +1025,7 @@ static int aac_read_block64(struct fib * fib, struct
> scsi_cmnd * cmd, u64 lba, u
> {
> u16 fibsize;
> struct aac_read64 *readcmd;
> + unsigned long ret;
> aac_fib_init(fib);
> readcmd = (struct aac_read64 *) fib_data(fib);
> readcmd->command = cpu_to_le32(VM_CtHostRead64);
> @@ -1028,7 +1035,9 @@ static int aac_read_block64(struct fib * fib, struct
> scsi_cmnd * cmd, u64 lba, u
> readcmd->pad = 0;
> readcmd->flags = 0;
>
> - aac_build_sg64(cmd, &readcmd->sg);
> + ret = aac_build_sg64(cmd, &readcmd->sg);
> + if (ret < 0)
> + return ret;
> fibsize = sizeof(struct aac_read64) +
> ((le32_to_cpu(readcmd->sg.count) - 1) *
>sizeof (struct sgentry64));
> @@ -1050,6 +1059,8 @@ static int aac_read_block(struct fib * fib, struct
> scsi_cmnd * cmd, u64 lba, u32
> {
> u16 fibsize;
> struct aac_read *readcmd;
> + unsigned long ret;
> +
> aac_fib_init(fib);
> readcmd = (struct aac_read *) fib_data(fib);
> readcmd->command = cpu_to_le32(VM_CtBlockRead);
> @@ -1057,7 +1068,9 @@ static int aac_read_block(struct fib * fib, struct
> scsi_cmnd * cmd, u64 lba, u32
> readcmd->block = cpu_to_le32((u32)(lba&0x));
> readcmd->count = cpu_to_le32(count * 512);
>
> - aac_build_sg(cmd, &readcmd->sg);
> + ret = aac_build_sg(cmd, &readcmd->sg);
> + if (ret < 0)
> + return ret;
> fibsize = sizeof(struct aac_read) +
> ((le32_to_cpu(readcmd->sg.count) - 1) *
>sizeof (struct sgentry));
> @@ -1079,6 +1092,7 @@ static int aac_write_raw_io(struct fib * fib, struct
> scsi_cmnd * cmd, u64 lba, u
> {
> struct aac_dev *dev = fib->dev;
> u16 fibsize, command;
> + unsigned long ret;
>
> aac_fib_init(fib);
> if (dev->comm_interface == AAC_COMM_MESSAGE_TYPE2 && !dev->sync_mode) {
> @@ -1093,7 +1107,10 @@ static int aac_write_raw_io(struct fib * fib, struct
> scsi_cmnd * cmd, u64 lba, u
> (((aac_cache & 5) != 5) ||
> !fib->dev->cache_protected)) ?
> cpu_to_le16(RIO2_IO_TYPE_WRITE|RIO2_IO_SUREWRITE) :
> cpu_to_le16(RIO2_IO_TYPE_WRITE);
> - aac_build_sgraw2(cmd, writecmd2,
> dev->scsi_host_ptr->sg_tablesize);
> + ret = aac_build_sgraw2(cmd, writecmd2,
> + dev->scsi_host_ptr->sg_tablesize);
> + if (ret < 0)
> + return ret;
>
[PATCH 1/1] aacraid: SCSI dma mapping failure case handling
This patch handles SCSI dma mapping failure case. Reporting error code to the
upper layer instead of BUG_ON().
This patch is created against current upstream kernel.
Signed-off-by: Mahesh Rajashekhara
---
drivers/scsi/aacraid/aachba.c | 63 +++-
drivers/scsi/aacraid/aacraid.h |2 +-
2 files changed, 50 insertions(+), 15 deletions(-)
diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index d79457a..efa2900 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -971,6 +971,7 @@ static int aac_read_raw_io(struct fib * fib, struct
scsi_cmnd * cmd, u64 lba, u3
{
struct aac_dev *dev = fib->dev;
u16 fibsize, command;
+ unsigned long ret;
aac_fib_init(fib);
if (dev->comm_interface == AAC_COMM_MESSAGE_TYPE2 && !dev->sync_mode) {
@@ -982,7 +983,10 @@ static int aac_read_raw_io(struct fib * fib, struct
scsi_cmnd * cmd, u64 lba, u3
readcmd2->byteCount = cpu_to_le32(count<<9);
readcmd2->cid = cpu_to_le16(scmd_id(cmd));
readcmd2->flags = cpu_to_le16(RIO2_IO_TYPE_READ);
- aac_build_sgraw2(cmd, readcmd2,
dev->scsi_host_ptr->sg_tablesize);
+ ret = aac_build_sgraw2(cmd, readcmd2,
+ dev->scsi_host_ptr->sg_tablesize);
+ if (ret < 0)
+ return ret;
command = ContainerRawIo2;
fibsize = sizeof(struct aac_raw_io2) +
((le32_to_cpu(readcmd2->sgeCnt)-1) * sizeof(struct
sge_ieee1212));
@@ -996,7 +1000,9 @@ static int aac_read_raw_io(struct fib * fib, struct
scsi_cmnd * cmd, u64 lba, u3
readcmd->flags = cpu_to_le16(RIO_TYPE_READ);
readcmd->bpTotal = 0;
readcmd->bpComplete = 0;
- aac_build_sgraw(cmd, &readcmd->sg);
+ ret = aac_build_sgraw(cmd, &readcmd->sg);
+ if (ret < 0)
+ return ret;
command = ContainerRawIo;
fibsize = sizeof(struct aac_raw_io) +
((le32_to_cpu(readcmd->sg.count)-1) * sizeof(struct
sgentryraw));
@@ -1019,6 +1025,7 @@ static int aac_read_block64(struct fib * fib, struct
scsi_cmnd * cmd, u64 lba, u
{
u16 fibsize;
struct aac_read64 *readcmd;
+ unsigned long ret;
aac_fib_init(fib);
readcmd = (struct aac_read64 *) fib_data(fib);
readcmd->command = cpu_to_le32(VM_CtHostRead64);
@@ -1028,7 +1035,9 @@ static int aac_read_block64(struct fib * fib, struct
scsi_cmnd * cmd, u64 lba, u
readcmd->pad = 0;
readcmd->flags = 0;
- aac_build_sg64(cmd, &readcmd->sg);
+ ret = aac_build_sg64(cmd, &readcmd->sg);
+ if (ret < 0)
+ return ret;
fibsize = sizeof(struct aac_read64) +
((le32_to_cpu(readcmd->sg.count) - 1) *
sizeof (struct sgentry64));
@@ -1050,6 +1059,8 @@ static int aac_read_block(struct fib * fib, struct
scsi_cmnd * cmd, u64 lba, u32
{
u16 fibsize;
struct aac_read *readcmd;
+ unsigned long ret;
+
aac_fib_init(fib);
readcmd = (struct aac_read *) fib_data(fib);
readcmd->command = cpu_to_le32(VM_CtBlockRead);
@@ -1057,7 +1068,9 @@ static int aac_read_block(struct fib * fib, struct
scsi_cmnd * cmd, u64 lba, u32
readcmd->block = cpu_to_le32((u32)(lba&0x));
readcmd->count = cpu_to_le32(count * 512);
- aac_build_sg(cmd, &readcmd->sg);
+ ret = aac_build_sg(cmd, &readcmd->sg);
+ if (ret < 0)
+ return ret;
fibsize = sizeof(struct aac_read) +
((le32_to_cpu(readcmd->sg.count) - 1) *
sizeof (struct sgentry));
@@ -1079,6 +1092,7 @@ static int aac_write_raw_io(struct fib * fib, struct
scsi_cmnd * cmd, u64 lba, u
{
struct aac_dev *dev = fib->dev;
u16 fibsize, command;
+ unsigned long ret;
aac_fib_init(fib);
if (dev->comm_interface == AAC_COMM_MESSAGE_TYPE2 && !dev->sync_mode) {
@@ -1093,7 +1107,10 @@ static int aac_write_raw_io(struct fib * fib, struct
scsi_cmnd * cmd, u64 lba, u
(((aac_cache & 5) != 5) ||
!fib->dev->cache_protected)) ?
cpu_to_le16(RIO2_IO_TYPE_WRITE|RIO2_IO_SUREWRITE) :
cpu_to_le16(RIO2_IO_TYPE_WRITE);
- aac_build_sgraw2(cmd, writecmd2,
dev->scsi_host_ptr->sg_tablesize);
+ ret = aac_build_sgraw2(cmd, writecmd2,
+ dev->scsi_host_ptr->sg_tablesize);
+ if (ret < 0)
+ return ret;
command = ContainerRawIo2;
fibsize = sizeof(struct aac_raw_io2) +
((le32_to_cpu(writecmd2->sgeCnt)-1) * sizeof(struct
sge_ieee1212));
@@ -1110,7 +1127,9 @@ static
[PATCH] mpt: missing break
From: Alan Cox This happens to do the right thing in all cases on fibre channel but not on other media types Signed-off-by: Alan Cox --- drivers/message/fusion/mptscsih.c |1 + 1 file changed, 1 insertion(+) diff --git a/drivers/message/fusion/mptscsih.c b/drivers/message/fusion/mptscsih.c index 0c3ced7..164afa7 100644 --- a/drivers/message/fusion/mptscsih.c +++ b/drivers/message/fusion/mptscsih.c @@ -792,6 +792,7 @@ mptscsih_io_done(MPT_ADAPTER *ioc, MPT_FRAME_HDR *mf, MPT_FRAME_HDR *mr) * than an unsolicited DID_ABORT. */ sc->result = DID_RESET << 16; + break; case MPI_IOCSTATUS_SCSI_EXT_TERMINATED: /* 0x004C */ if (ioc->bus_type == FC) -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
