Matthew Wilcox wrote:
> On Sun, Mar 08, 2009 at 12:28:27PM +0200, Boaz Harrosh wrote:
>> That's because you are doing it at the wrong level at the wrong stage.
>> 1. block-level submits a request
>> 2. sd/sr or what ever ULD prepares a scsi_cmnd out of request.
>> Request's sizes are only a recommendation. ULD or scsi-ml may
>> prepare a smaller command then request. Once command is prepared
>> request is disregarded, you can bang on it all you want code will
>> not care about it one bit.
>
> I may well be changing more than I need to, but it would be foolish of
> me to make the assumption at this point that nothing is looking at the
> request.
>
>> 3. LLD executes the scsi-command (Not the block-request)
>
> This is true, *but* some of the lengths in the block request still end
> up getting used, for example bv_len is used by blk_rq_map_sg() which is
> called by the LLD.
>
I must be running but... What? blk_rq_map_sg() is called by scsi-ml in prep_command
when allocating and setting up SGs (scsi_sglist())
If called by LLD then it's the first I've herd of, and some assumptions at scsi-ml
surly break.
This is exactly stage (2) the call to blk_rq_map_sg(), from there on request is
ignored.
>> 4. scsi-ml completes command's bytes, at this stage the request might
>> not be over and, and a reminder is re-prepared so the request can
>> be complete.
>>
>> The code above scmd->sdb.length = req->data_len = size; is not allowed
>> and can cause data leaks.
>
> Simply not true. We are *changing the amount of data we wish to
> transfer*. SCSI would have sent down 24 bytes of data. ATA needs to
> send down 512 bytes of data.
>
That's what you want but you can't do that after stage 2.
>> You should ping Tejun, block-layer(1) and ATA-LLD(3) has a way to communicate
>> alignments and drain buffers that expose some other possible lenght's to ata.
>>
>> And to your question the missing length above is probably encoded inside the
>> submitted CDB. (scsi_cmnd->cmnd). When you change the length before
>> stage (2) it works.
>
> No, that's not it. This works (in sd.c):
>
> if (bio_add_pc_page(q, bio, page, 512, 0) < 512) {
> [...]
> rq->cmd_type = REQ_TYPE_BLOCK_PC;
> rq->cmd_len = 10;
> rq->cmd[0] = UNMAP;
> put_unaligned_be16(24, rq->cmd + 7);
>
> So the 24 in the cmd gets ignored.
>
>> I think you should be using the drain mechanisms built into ata
>
> drain seems only applicable to ATAPI, not to ATA. The comment says:
>
> * ATAPI commands which transfer variable length data to host
> * might overflow due to application error or hardare bug. This
> * function checks whether overflow should be drained and ignored
> * for @request.
>
> This isn't the case with discard/UNMAP/TRIM. We know exactly how much
> data we want to send. The problem is that I don't know how to update all
> the required places to change the amount of data being sent. I don't
> see any other ATA command which needs to do this, so this is breaking
> new ground for libata-scsi.
>
You must do the change before/during stage 2 above. (length get encoded inside
scsi_sglist()).
Sorry I must be running. If you have a public git with this stuff
I can have a look tomorrow, or send a .diff I'll apply locally and
inspect it.
bye
Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
