Re: [PATCH] SQUASHME: Streamline pmem.c

[Christoph Hellwig]

On Thu, Mar 26, 2015 at 07:02:17PM +0200, Boaz Harrosh wrote:
> Christoph why did you choose the fat and ugly version of
> pmem.c beats me. Anyway, here are the cleanups you need on
> top of your pmem patch.
> 
> Among other it does:
> * Remove getgeo. It is not needed for modern fdisk and was never
>   needed for libgparted and cfdisk.
> 
> * remove 89 lines of code to do a single memcpy. The reason
>   this was so in brd (done badly BTW) is because destination
>   memory is page-by-page based. With pmem we have the destination
>   contiguous so we can do any size, in one go.
> 
> * Remove SECTOR_SHIFT. It is defined in 6 other places
>   in the Kernel. I do not like a new one. 9 is used through
>   out, including block core. I do not like pmem to blasphemy
>   more than needed.
> 
> * More style stuff ...

One patch per items please..

> - * This driver is heavily based on drivers/block/brd.c.
> + * This driver's skeleton is based on drivers/block/brd.c.
>   * Copyright (C) 2007 Nick Piggin
>   * Copyright (C) 2007 Novell Inc.

Looks like there is basically nothing left of brd.c after this patch,
so we might as well drop this.

> -/*
> - * direct translation from (pmem,sector) => void*
> - * We do not require that sector be page aligned.
> - * The return value will point to the beginning of the page containing the
> - * given sector, not to the sector itself.
> - */

not quite related to you patch:  all the pmem and direct_access code uses
normal kernel address pointers, but we're actually dealing with iomem
here which makes sparse a little unhappy..

> -	BUG_ON(bio->bi_rw & REQ_DISCARD);
> +	if (WARN_ON(bio->bi_rw & REQ_DISCARD)) {
> +		err = -EINVAL;
> +		goto out;
> +	}

No need to write additional code here, I'd rather remove it entirely
if the BUG_ON bothers you.  There is no way we'll get a discard without
the driver asking for it.  And then you'd have to check for all the
other non-standard I/O types as well.

> +		/* NOTE: There is a legend saying that bv_len might be
> +		 * bigger than PAGE_SIZE in the case that bv_page points to
> +		 * a physical contiguous PFN set. But for us it is fine because
> +		 * it means the Kernel virtual mapping is also contiguous. And
> +		 * on the pmem side we are always contiguous both virtual and
> +		 * physical
> +		 */

Linux comment style has the opening "/*" on it's own line.  And talking
about legends in comments isn't a very nice style either.
--
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/