Re: Getting TRIM working

[Matthew Wilcox <matthew@wil.cx>]

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.

> 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.

> 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.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."