Re: [PATCH 1/6] compcache: compressed RAM block device

Nitin Gupta
2008-03-24T18:35:35Z

On Mon, Mar 24, 2008 at 9:53 PM, Will Newton <will.newton@gmail.com> wrote:
>
> On Mon, Mar 24, 2008 at 3:02 PM, Nitin Gupta <nitingupta910@gmail.com> wrote:

> > diff --git a/drivers/block/Makefile b/drivers/block/Makefile
> > index 5e58430..b6d3dd2 100644
> > --- a/drivers/block/Makefile
> > +++ b/drivers/block/Makefile
> > @@ -12,6 +12,7 @@ obj-$(CONFIG_PS3_DISK) += ps3disk.o
> > obj-$(CONFIG_ATARI_FLOPPY) += ataflop.o
> > obj-$(CONFIG_AMIGA_Z2RAM) += z2ram.o
> > obj-$(CONFIG_BLK_DEV_RAM) += brd.o
> > +obj-$(CONFIG_BLK_DEV_COMPCACHE) += compcache.o
>
> Minor - this isn't in alphabetical order.

Intent here is to keep related things together. So, I have placed it
with generic ramdisk. This also seems to be convention used in this
file.

> > diff --git a/drivers/block/compcache.c b/drivers/block/compcache.c
> > new file mode 100644
> > index 0000000..4ffcd63
> > --- /dev/null
> > +++ b/drivers/block/compcache.c
> > @@ -0,0 +1,440 @@
> > +/*
> > + * Compressed RAM based swap device
> > + *
> > + * (C) Nitin Gupta
>
> Should add a copyright year.
>

ok.

> > +#include <asm/string.h>
>
> Should this be <linux/string.h>?
>

Yes. I will change this.

> > +/* Check if request is within bounds and page aligned */
> > +static inline int valid_swap_request(struct bio *bio)
> > +{
> > + if (unlikely((bio->bi_sector >= compcache.size) ||
> > + (bio->bi_sector & (SECTORS_PER_PAGE - 1)) ||
> > + (bio->bi_vcnt != 1) ||
> > + (bio->bi_size != PAGE_SIZE) ||
> > + (bio->bi_io_vec[0].bv_offset != 0)))
> > + return 0;
> > + return 1;
> > +}
>
> Probably unnecessary to mark this explicitly inline.
>
>

Probably yes. I am not sure.

> > + /*
> > + * It is named like this to prevent distro installers
> > + * from offering compcache as installation target. They
> > + * seem to ignore all devices beginning with 'ram'
> > + */
> > + sprintf(compcache.disk->disk_name, "%s", "ramzswap0");
>
> I'm not sure the name makes it 100% obvious what the device is for.
> You could use strcpy here also.
>

"z" == compress
and hence the name ramzswap :)

> > + if (compcache.table[0].addr)
> > + free_page((unsigned long)compcache.table[0].addr);
> > + if (compcache.compress_workmem)
> > + kfree(compcache.compress_workmem);
> > + if (compcache.compress_buffer)
> > + kfree(compcache.compress_buffer);
> > + if (compcache.table)
> > + vfree(compcache.table);
>
> kfree() and vfree() may safely be called on NULL pointers.
>

I will remove these unnecessary checks then.

Thanks,
Nitin