[I thought I replied to this, but I don't see an indication that I did]
On Sun, Apr 05, 2009 at 03:28:07PM +0300, Boaz Harrosh wrote:
> > +++ b/block/blk-barrier.c
> > @@ -356,6 +356,8 @@ static void blkdev_discard_end_io(struct bio *bio, int err)
> > clear_bit(BIO_UPTODATE, &bio->bi_flags);
> > }
> >
> > + if (bio_has_data(bio))
> > + __free_page(bio_page(bio));
>
> Page freed which was allocated by the LLD
It wasn't allocated by the LLD. It was allocated by the ULD.
> > bio_put(bio);
>
> OK bio was allocated by user code but shouldn't
? Are you saying the bio should be allocated by each driver
implementing a discard operation?
> > while (nr_sects && !ret) {
> > - bio = bio_alloc(gfp_mask, 0);
> > + bio = bio_alloc(gfp_mask, 1);
>
> blkdev_issue_discard() and blk_ioctl_discard() has half a page
> of common (and changing) code, could be done to use a common
> helper that sets policy about bio allocation sizes and such.
>
> Just my $0.017
Yes, that works nicely. Thanks for the suggestion.
> > @@ -1118,7 +1120,7 @@ void init_request_from_bio(struct request *req, struct bio *bio)
> > req->cmd_flags |= REQ_DISCARD;
> > if (bio_barrier(bio))
> > req->cmd_flags |= REQ_SOFTBARRIER;
> > - req->q->prepare_discard_fn(req->q, req);
> > + req->q->prepare_discard_fn(req->q, req, bio);
>
> Allocation of bio page could be done commonly here.
> The prepare_discard_fn() is made to return the needed size. It is not as if we actually
> give the driver a choice about the allocation.
Not all drivers need to allocate a page. Some drivers may need
to allocate more than one page, depending on how large the range is.
And the driver can't just return the page size it needs here -- it needs
to fill in the contents of the page too.
I suppose we could do something fairly disgusting like:
for (;;) {
struct page *page;
needed = req->q->prepare_discard_fn(req->q, req, bio);
if (!needed)
break;
page = alloc_page(GFP_KERNEL);
if (bio_add_pc_page(q, bio, page, PAGE_SIZE, 0) < PAGE_SIZE)
goto fail;
}
Then the driver can return 0 => success, anything else => allocate more
ram, try again.
> I have one question:
>
> At [PATCH 4/5] and [PATCH 4/5] you do:
> + struct page *page = alloc_page(GFP_KERNEL);
>
> does that zero the alloced page? since if I understand correctly this page
> will go on the wire, a SW target on the other size could snoop random Kernel
> memory, is that allowed? OK I might be totally clueless here.
alloc_page doesn't zero the page.
scsi only sends out 24 bytes of that page on the wire, and it initialises
all 24 bytes. ide/ata send multiples of 512 bytes on the wire, and
they're careful to zero any of the space they're not using.
--
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
