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

[Nitin Gupta]

On Mon, Mar 24, 2008 at 9:53 PM, Will Newton <will.newton@xxxxxxxxx> wrote:
>
> On Mon, Mar 24, 2008 at 3:02 PM, Nitin Gupta <nitingupta910@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/