From: Christoph Hellwig <hch@lst.de>
To: Boaz Harrosh <boaz@plexistor.com>
Cc: Christoph Hellwig <hch@lst.de>,
linux-nvdimm@ml01.01.org, linux-fsdevel@vger.kernel.org,
linux-kernel@vger.kernel.org, x86@kernel.org,
ross.zwisler@linux.intel.com, axboe@kernel.dk
Subject: Re: [PATCH] SQUASHME: Streamline pmem.c
Date: Thu, 26 Mar 2015 18:23:38 +0100 [thread overview]
Message-ID: <20150326172338.GB25575@lst.de> (raw)
In-Reply-To: <55143B99.7060407@plexistor.com>
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.
next prev parent reply other threads:[~2015-03-26 17:23 UTC|newest]
Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-26 8:32 another pmem variant V2 Christoph Hellwig
2015-03-26 8:32 ` [PATCH 1/3] pmem: Initial version of persistent memory driver Christoph Hellwig
2015-03-26 14:12 ` [Linux-nvdimm] " Dan Williams
2015-03-26 14:35 ` Christoph Hellwig
2015-03-26 21:37 ` Ross Zwisler
2015-03-26 14:52 ` Boaz Harrosh
2015-03-26 15:59 ` Dan Williams
2015-03-26 8:32 ` [PATCH 2/3] x86: add a is_e820_ram() helper Christoph Hellwig
2015-03-26 9:02 ` Ingo Molnar
2015-03-26 9:34 ` Christoph Hellwig
2015-03-26 10:04 ` Ingo Molnar
2015-03-26 10:19 ` Christoph Hellwig
2015-03-26 10:28 ` Ingo Molnar
2015-03-26 10:29 ` Christoph Hellwig
2015-03-26 15:49 ` Boaz Harrosh
2015-03-26 16:02 ` [Linux-nvdimm] " Dan Williams
2015-03-26 16:07 ` Boaz Harrosh
2015-03-26 16:43 ` Christoph Hellwig
2015-03-26 18:46 ` Elliott, Robert (Server Storage)
2015-03-26 19:25 ` [Linux-nvdimm] " Dan Williams
2015-03-26 20:53 ` Ross Zwisler
2015-03-26 22:59 ` Yinghai Lu
2015-03-27 8:10 ` Christoph Hellwig
2015-03-26 8:32 ` [PATCH 3/3] x86: add support for the non-standard protected e820 type Christoph Hellwig
2015-03-26 16:57 ` another pmem variant V2 Boaz Harrosh
2015-03-26 17:02 ` [PATCH] SQUASHME: Streamline pmem.c Boaz Harrosh
2015-03-26 17:23 ` Christoph Hellwig [this message]
2015-03-26 22:17 ` Ross Zwisler
2015-03-26 22:22 ` Ross Zwisler
2015-03-26 23:31 ` [Linux-nvdimm] " Dan Williams
2015-03-31 13:44 ` Boaz Harrosh
2015-03-26 17:18 ` another pmem variant V2 Christoph Hellwig
2015-03-26 17:31 ` Boaz Harrosh
2015-03-26 18:38 ` Christoph Hellwig
2015-03-31 9:25 ` Christoph Hellwig
2015-03-31 10:25 ` Boaz Harrosh
2015-03-31 10:31 ` Boaz Harrosh
2015-03-31 14:21 ` [RFC] SQUASHME: pmem: Split up pmem_probe from pmem_alloc Boaz Harrosh
2015-03-31 16:10 ` Christoph Hellwig
2015-03-31 16:08 ` another pmem variant V2 Christoph Hellwig
2015-03-31 13:18 ` [SQUASHME 0/6] Streamline of Initial pmem submission Boaz Harrosh
2015-03-31 13:23 ` [PATCH 1/6] SQUASHME: Don't let e820_PMEM sections Boaz Harrosh
2015-03-31 17:16 ` [Linux-nvdimm] " Brooks, Adam J
2015-03-31 13:24 ` [PATCH 2/6] SQUASHME: pmem: Remove getgeo Boaz Harrosh
2015-03-31 13:25 ` [PATCH 3/6] SQUASHME: pmem: Streamline pmem driver Boaz Harrosh
2015-03-31 13:27 ` [PATCH 4/6] SQUSHME: pmem: Micro cleaning Boaz Harrosh
2015-03-31 15:17 ` [Linux-nvdimm] " Dan Williams
2015-03-31 15:24 ` Boaz Harrosh
2015-03-31 15:30 ` Dan Williams
2015-03-31 15:43 ` Boaz Harrosh
2015-03-31 19:40 ` Matthew Wilcox
2015-03-31 13:28 ` [PATCH 5/6] SQUASHME: pmem: Remove SECTOR_SHIFT Boaz Harrosh
2015-03-31 13:33 ` [PATCH 6/6] SQUASHME: pmem: Remove "... based on brd.c" + Copyright Boaz Harrosh
2015-03-31 15:14 ` another pmem variant V2 Boaz Harrosh
2015-03-31 16:16 ` Christoph Hellwig
2015-03-31 16:44 ` Ingo Molnar
2015-03-31 17:24 ` Christoph Hellwig
2015-03-31 17:33 ` [Linux-nvdimm] " Dan Williams
2015-04-01 7:50 ` Ingo Molnar
2015-04-01 8:06 ` Boaz Harrosh
2015-04-01 12:49 ` Boaz Harrosh
2015-03-31 22:11 ` Elliott, Robert (Server Storage)
2015-04-01 7:26 ` Christoph Hellwig
2015-04-02 15:11 ` Elliott, Robert (Server Storage)
2015-04-02 16:41 ` Christoph Hellwig
2015-04-02 18:03 ` Ingo Molnar
2015-04-01 19:33 ` Elliott, Robert (Server Storage)
2015-04-02 9:37 ` Christoph Hellwig
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150326172338.GB25575@lst.de \
--to=hch@lst.de \
--cc=axboe@kernel.dk \
--cc=boaz@plexistor.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nvdimm@ml01.01.org \
--cc=ross.zwisler@linux.intel.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).