From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752429AbbCZRXm (ORCPT ); Thu, 26 Mar 2015 13:23:42 -0400 Received: from verein.lst.de ([213.95.11.211]:53335 "EHLO newverein.lst.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750819AbbCZRXk (ORCPT ); Thu, 26 Mar 2015 13:23:40 -0400 Date: Thu, 26 Mar 2015 18:23:38 +0100 From: Christoph Hellwig To: Boaz Harrosh Cc: Christoph Hellwig , 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 Message-ID: <20150326172338.GB25575@lst.de> References: <1427358764-6126-1-git-send-email-hch@lst.de> <55143A8B.2060304@plexistor.com> <55143B99.7060407@plexistor.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55143B99.7060407@plexistor.com> User-Agent: Mutt/1.5.17 (2007-11-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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.