Re: New TRIM/UNMAP tree published (2009-05-02)

[Boaz Harrosh <bharrosh@panasas.com>]

On 05/03/2009 05:48 PM, Matthew Wilcox wrote:
> On Sun, May 03, 2009 at 02:07:32PM +0100, Hugh Dickins wrote:
>> I notice OCZ hope to have updated Vertex firmware mid-May,
>> such that their TRIM support matches up with (y)ours.
> 
> Excellent.
> 
>> You have GFP_KERNEL allocations in your discard_fn()s.  I believe that
>> allocations down at that level will need to be GFP_NOIO - the bio mempool
>> might be empty by this point, you don't want memory allocation to get into
>> submitting even more I/O to satisfy your discard_fn().  Does it need its
>> own mempool of reserve pages?  My guess is that you can get away without
>> that, since a failed discard, though regrettable, loses no data.
> 
> You make a good point.  There's definitely still work to be done around
> error handling.  OTOH, we could always do what IDE does ...
> 
> static void idedisk_prepare_flush(struct request_queue *q, struct request *rq)
> {
>         ide_drive_t *drive = q->queuedata;
>         struct ide_cmd *cmd = kmalloc(sizeof(*cmd), GFP_ATOMIC);
> 
>         /* FIXME: map struct ide_taskfile on rq->cmd[] */
>         BUG_ON(cmd == NULL);
> 
> 
> 

Out from the frying pan and into the fire. ;-)

I agree with Hugh. The allocation is done at, too-low in the food chain.
(And that free of buffer at upper layer allocated by lower layer).

I think you need to separate the: "does lld need buffer, what size"
from the "here is buffer prepare", so upper layer that can sleep does
sleep.

In all other buffer needing operations the allocation is done before
submission of request, No?

Just my $0.017
Thanks
Boaz

BTW:
 I've not have time to review second round will "TODO"