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-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
