Re: [PATCH/RFC] ps3/block: Add ps3vram-ng driver for accessing video RAM as block device

On Thu, Mar 05 2009, Geert Uytterhoeven wrote:

	Hi Jens,

On Thu, 5 Mar 2009, Jens Axboe wrote:

quoted

On Wed, Mar 04 2009, Geert Uytterhoeven wrote:

quoted

Below is the rewrite of the PS3 Video RAM Storage Driver as a plain block
device, as requested by Arnd Bergmann.

The MTD-based PS3 Video RAM Storage Driver was integrated into the mainline
kernel in 2.6.29-rc1.

Ideally, we think it would be best if the existing MTD-based ps3vram driver
would be replaced by the new block-based ps3vram driver before 2.6.29 is
released. This would relieve the burden of supporting two different swap space
schemes on PS3 (swap on /dev/mtdblock0 vs. /dev/ps3vram) from the distro
maintainer's shoulders, as in that case there would never have been a stable
kernel version containing the MTD-based ps3vram driver.

What do you think? If this is accepted, I'll submit a patch to remove the MTD
ps3vram and add the new driver as ps3vram (instead of ps3vram-ng).

Thanks for your (review and other) comments!

I'd rewrite this as a ->make_request_fn handler instead. Then you can
get rid of the kernel thread. IOW, change

queue = blk_init_queue(ps3vram_request, &priv->lock);

to

queue = blk_alloc_queue(GFP_KERNEL);
blk_queue_make_request(queue, ps3vram_make_request);

Thanks, I didn't know that part...

quoted

Add error handling of course, and call blk_queue_max_*() to set your
limits for this device.

I took out the blk_queue_max_*() calls (compared to ps3disk.c), as
none of the limits apply, and the defaults are fine.

Is that OK, or is it better to make it explicit?

I think it's always good to make it explicit. Plus for this case you
definitely need it, as blk_init_queue() wont do it for you anymore.

quoted

Then add a ps3vram_make_request() ala:

quoted

static void ps3vram_do_request(struct request_queue *q, struct bio *bio)
{
	struct ps3_system_bus_device *dev = q->queuedata;
	struct ps3vram_priv *priv = dev->core.driver_data;
	int write, res, err = -EIO;
        struct bio_vec *bv;
	sector_t sector;
	loff_t offset;
	size_t len, retlen;
        int i;

	write = bio_data_dir(bio) == WRITE;

	sector = bio->bi_sector;
        bio_for_each_segment(bv, bio, i) {
                char *ptr = page_address(bv->bv_page) + bv->bv_offset;

                len = bv->bv_len;
	        offset = sector << 9;

        	if (write)
	        	res = ps3vram_write(dev, offset, len, &retlen, ptr);
        	else
	        	res = ps3vram_read(dev, offset, len, &retlen, ptr);

	        if (res) {
        		dev_err(&dev->core, "%s failed\n", op);
	        	goto out;
        	}

        	if (retlen != len) {
	        	dev_err(&dev->core, "Short %s\n", op);
		        goto out;
        	}
                sector += (len >> 9);
        }

	dev_dbg(&dev->core, "%s completed\n", op);
        err = 0;
out:
        bio_endio(bio, err);
}

I just typed it here, so if it doesn't compile you get to keep the
pieces :-)

OK, I'll give it a try...

BTW, does this mean the `simple' way, which I used based on LDD3, is
deprecated?

Depends.. It's obviously not a very effective approach, since you punt
to a thread for each request. But if you need the IO scheduler helping
you with merging and sorting (for a rotational device), it still has
some merit. For this particular case, the ->make_request_fn approach is
much better.

quoted

Since ps3 is very RAM limited, I didn't bother with any highmem mapping
for the bio, since I gather that isn't an issue on that platform. You
may want to detail that in a comment above the page_addres() thing at
the top of the loop, though.

PS3 is ppc64, so no highmem. But I guess I best add the code for that anyway,
in case people will copy the driver in the future (I remember receiving a
similar comment for ps3disk).

I'd prefer just the comment, since you have to be able to handle
disabled interrupts on return from bvec_kmap_irq() if you use that. And
that would just complicate your driver, for something that you don't
need. So I'd put the comment in there stating why it's OK to just use
page_address() instead. If people copy-paste the code to some other
driver, then they will get the comment as well.

-- 
Jens Axboe