Re: [PATCH 3/4] aacraid: use blk_mq_rq_busy_iter() for traversing outstanding commands

[Christoph Hellwig <hch@lst.de>]

Dave and Sagar have been maintaining aacraid for a while, you should
Cc them.

This patch seems to touch fout entirely different areas in aacraid, it
would probably help to split it up into one patch per area explaining how
the replacement for the cmd list for choosen.

> +struct synchronize_busy_data {
> +	struct scsi_device *sdev;
> +	u64 lba;
> +	u32 count;
> +	int active;
> +};
> +
> +static bool synchronize_busy_iter(struct request *req, void *data, bool reserved)

This adds an overly long line. 

> +{
> +	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
> +	struct synchronize_busy_data *busy_data = data;
> +
> +	if (busy_data->sdev == cmd->device &&

Given that you itere over just a single scsi device using
blk_mq_queue_tag_busy_iter here would seem like the better API.

> +	    cmd->SCp.phase == AAC_OWNER_FIRMWARE) {

And the function would become more readable if it just exists early
for not firmware owned commands, as that saves one level of indentation
for all the heavy lifting.

> +		u64 cmnd_lba;
> +		u32 cmnd_count;
> +
> +		if (cmd->cmnd[0] == WRITE_6) {
> +			cmnd_lba = ((cmd->cmnd[1] & 0x1F) << 16) |
> +				(cmd->cmnd[2] << 8) |
> +				cmd->cmnd[3];
> +			cmnd_count = cmd->cmnd[4];
> +			if (cmnd_count == 0)
> +				cmnd_count = 256;
> +		} else if (cmd->cmnd[0] == WRITE_16) {

Instead of reverse engineering the lba and commands, why not check
the request for REQ_OP_WRITE and then look at bi_iter.bi_sector
(also for the caller to avoid the Linux sector to LBA conversion).

> +		if (((cmnd_lba + cmnd_count) < busy_data->lba) ||
> +		    (busy_data->count && ((busy_data->lba + busy_data->count) < cmnd_lba)))

Lots of braces not required here, and overy longs lines.

> +			return true;
> +		++busy_data->active;

Normally we do a post-fix increment if no one cares about the
return value.

> +	blk_mq_tagset_busy_iter(&sdev->host->tag_set, synchronize_busy_iter, &busy_data);

Another overly long line.

> +static bool wait_for_io_iter(struct request *rq, void *data, bool reserved)
> +{
> +	struct scsi_cmnd *command = blk_mq_rq_to_pdu(rq);
> +	int *active = data;
> +
> +	if (command->SCp.phase == AAC_OWNER_FIRMWARE)
> +		*active = 1;
> +	return true;
> +}

Without bloc ayer quiescing this use is a bit of a hack.  Can you
add a comment toward that?

> +static bool reset_adapter_iter(struct request *rq, void *data, bool reserved)
> +{
> +	struct scsi_cmnd *command = blk_mq_rq_to_pdu(rq);
> +
> +	if (command->SCp.phase == AAC_OWNER_FIRMWARE) {
> +		command->result = DID_OK << 16
> +		  | COMMAND_COMPLETE << 8
> +		  | SAM_STAT_TASK_SET_FULL;

The | goes onto the previous line.

> +static int get_num_of_incomplete_fibs(struct aac_dev *aac)
> +{
>  	struct Scsi_Host *shost = aac->scsi_host_ptr;
>  	struct device *ctrl_dev;
> +	struct fib_count_data fib_count = {
> +		.mlcnt  = 0,
> +		.llcnt  = 0,
> +		.ehcnt  = 0,
> +		.fwcnt  = 0,
> +		.krlcnt = 0,
> +	};

You can do a:

	struct fib_count_data fib_count = { };

to zero all values.


> +	dev_info(ctrl_dev, "outstanding cmd: error handler-%d\n", fib_count.ehcnt);

This adds an overly long line.

> +	return fib_count.mlcnt + fib_count.llcnt + fib_count.ehcnt + fib_count.fwcnt;

Another one.