On Thu, Apr 27 2017 at 11:13am -0400,
Hannes Reinecke <hare@xxxxxxx> wrote:
> On 04/27/2017 05:11 PM, Bart Van Assche wrote:
> > On Thu, 2017-04-27 at 07:46 +0200, Hannes Reinecke wrote:
> >> On 04/26/2017 08:37 PM, Bart Van Assche wrote:
> >>> + clone = blk_get_request(q, rq->cmd_flags | REQ_NOMERGE, GFP_ATOMIC);
> >>> if (IS_ERR(clone)) {
> >>> /* EBUSY, ENODEV or EWOULDBLOCK: requeue */
> >>> - return r;
> >>> + pr_debug("blk_get_request() returned %ld%s - requeuing\n",
> >>> + PTR_ERR(clone), blk_queue_dying(q) ?
> >>> + " (path offline)" : "");
> >>> + if (blk_queue_dying(q)) {
> >>> + atomic_inc(&m->pg_init_in_progress);
> >>> + activate_path(pgpath);
> >>> + return DM_MAPIO_REQUEUE;
> >>> + }
> >>> + return DM_MAPIO_DELAY_REQUEUE;
> >>> }
> >>> clone->bio = clone->biotail = NULL;
> >>> clone->rq_disk = bdev->bd_disk;
> >>
> >> At the very least this does warrant some inline comments.
> >> Why do we call activate_path() here, seeing that the queue is dying?
> >
> > Hello Hannes,
> >
> > activate_path() is not only able to activate a path but can also change
> > the state of a path to offline. The body of the activate_path() function
> > makes that clear and that is why I had not added a comment above the
> > activate_path() call:
> >
> > static void activate_path(struct pgpath *pgpath)
> > {
> > struct request_queue *q = bdev_get_queue(pgpath->path.dev->bdev);
> >
> > if (pgpath->is_active && !blk_queue_dying(q))
> > scsi_dh_activate(q, pg_init_done, pgpath);
> > else
> > pg_init_done(pgpath, SCSI_DH_DEV_OFFLINED);
> > }
> >
> So why not call 'pg_init_done()' directly and avoid the confusion?
Doing so is sprinkling more SCSI specific droppings in code that should
be increasingly transport agnostic. Might be worth renaming
activate_path() to activate_or_offline_path() ?
Mike