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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 03 May 2009 18:02:51 +0300
From:	Boaz Harrosh <bharrosh@...asas.com>
To:	Matthew Wilcox <matthew@....cx>
CC:	Hugh Dickins <hugh@...itas.com>,
	Matthew Wilcox <willy@...ux.intel.com>,
	linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org,
	Jeff Garzik <jgarzik@...hat.com>, linux-scsi@...r.kernel.org,
	Jens Axboe <jens.axboe@...cle.com>,
	Bartlomiej Zolnierkiewicz <bzolnier@...il.com>,
	Mark Lord <lkml@....ca>
Subject: Re: New TRIM/UNMAP tree published (2009-05-02)

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"
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/