linux-scsi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Bart Van Assche <bvanassche@acm.org>
To: Tejun Heo <tj@kernel.org>
Cc: linux-scsi <linux-scsi@vger.kernel.org>,
	James Bottomley <jbottomley@parallels.com>,
	Mike Christie <michaelc@cs.wisc.edu>,
	Jens Axboe <axboe@kernel.dk>, Chanho Min <chanho.min@lge.com>,
	Hannes Reinecke <hare@suse.de>
Subject: Re: [PATCH v6 10/13] Make scsi_remove_host() wait for device removal
Date: Fri, 07 Dec 2012 08:41:52 +0100	[thread overview]
Message-ID: <50C19DC0.8040000@acm.org> (raw)
In-Reply-To: <20121203164257.GE19802@htj.dyndns.org>

On 12/03/12 17:42, Tejun Heo wrote:
> On Mon, Dec 03, 2012 at 05:38:52PM +0100, Bart Van Assche wrote:
>> It is indeed possible to invoke complete() only if the device list
>> became empty with the host state equal to SHOST_CANCEL,
>> SHOST_CANCEL_RECOVERY, SHOST_DEL or SHOST_DEL_RECOVERY and in
>
> We can test this with !scsi_host_scan_allowed(), right?
>
>> scsi_remove_host() to wait for that completion only if the device
>> list was not empty before the host state was changed into one of the
>> four mentioned states. Do you really prefer this approach over the
>> approach in the patch at the start of this thread ?
>
> Maybe I'm missing something but if possible completion tends to be
> much simpler than using waitqueue directly.  I *think* that's the case
> here.  Am I missing something?

Hello Tejun,

You are right that it should be possible to use a completion in this 
context instead of a wait queue. However, I'm not enthusiast about using 
a completion for this patch because it would mean introducing several 
additional if-statements. Such if-statements would make this patch even 
harder to test then it already is now.

Note: all your other review comments should have been addressed in v7 of 
this patch series.

Bart.


  reply	other threads:[~2012-12-07  7:41 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-11-28 12:39 [PATCH 0/13 v6] More device removal fixes Bart Van Assche
2012-11-28 12:42 ` [PATCH v6 01/13] block: Rename queue dead flag Bart Van Assche
2012-11-28 12:43 ` [PATCH v6 02/13] block: Let blk_drain_queue() caller obtain the queue lock Bart Van Assche
2012-11-28 12:44 ` [PATCH v6 03/13] block: Avoid that request_fn is invoked on a dead queue Bart Van Assche
2012-12-02 13:23   ` Tejun Heo
2012-12-02 13:35     ` Bart Van Assche
2012-11-28 12:45 ` [PATCH v6 04/13] block: Avoid scheduling delayed work " Bart Van Assche
2012-12-02 13:26   ` Tejun Heo
2012-12-02 13:41     ` Bart Van Assche
2012-12-02 13:59       ` Tejun Heo
2012-11-28 12:46 ` [PATCH v6 05/13] block: Make blk_cleanup_queue() wait until request_fn finished Bart Van Assche
2012-12-02 13:28   ` Tejun Heo
2012-11-28 12:47 ` [PATCH v6 06/13] bsg: Remove unused function bsg_goose_queue() Bart Van Assche
2012-12-02 13:29   ` Tejun Heo
2012-11-28 12:48 ` [PATCH v6 07/13] Fix race between starved list processing and device removal Bart Van Assche
2012-12-02 13:32   ` Tejun Heo
2012-11-28 12:48 ` [PATCH v6 08/13] Remove get_device() / put_device() pair from scsi_request_fn() Bart Van Assche
2012-12-02 13:34   ` Tejun Heo
2012-11-28 12:50 ` [PATCH v6 09/13] Avoid saving/restoring interrupt state inside scsi_remove_host() Bart Van Assche
2012-12-02 13:35   ` Tejun Heo
2012-11-28 12:51 ` [PATCH v6 10/13] Make scsi_remove_host() wait for device removal Bart Van Assche
2012-12-02 13:45   ` Tejun Heo
2012-12-02 13:48     ` Tejun Heo
2012-12-03  8:23     ` Bart Van Assche
2012-12-03 16:15       ` Tejun Heo
2012-12-03 16:38         ` Bart Van Assche
2012-12-03 16:42           ` Tejun Heo
2012-12-07  7:41             ` Bart Van Assche [this message]
2012-11-28 12:52 ` [PATCH v6 11/13] Make scsi_remove_host() wait until error handling finished Bart Van Assche
2012-12-02 13:51   ` Tejun Heo
2012-11-28 12:53 ` [PATCH v6 12/13] Avoid that scsi_device_set_state() triggers a race Bart Van Assche
2012-12-02 13:53   ` Tejun Heo
2012-11-28 12:53 ` [PATCH v6 13/13] Do not queue new I/O after scsi_remove_host() started Bart Van Assche
2012-12-02 13:58   ` Tejun Heo
2012-12-02 14:02 ` [PATCH 0/13 v6] More device removal fixes Tejun Heo
2012-12-06 13:33 ` Jens Axboe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=50C19DC0.8040000@acm.org \
    --to=bvanassche@acm.org \
    --cc=axboe@kernel.dk \
    --cc=chanho.min@lge.com \
    --cc=hare@suse.de \
    --cc=jbottomley@parallels.com \
    --cc=linux-scsi@vger.kernel.org \
    --cc=michaelc@cs.wisc.edu \
    --cc=tj@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).