linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Christoph Hellwig <hch@lst.de>
To: Dan Williams <dan.j.williams@intel.com>
Cc: linux-kernel@vger.kernel.org, Boaz Harrosh <boaz@plexistor.com>,
	Jan Kara <jack@suse.cz>, Mike Snitzer <snitzer@redhat.com>,
	Neil Brown <neilb@suse.de>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Heiko Carstens <heiko.carstens@de.ibm.com>,
	Chris Mason <clm@fb.com>, Paul Mackerras <paulus@samba.org>,
	"H. Peter Anvin" <hpa@zytor.com>,
	j.glisse@gmail.com, mingo@kernel.org,
	Alasdair Kergon <agk@redhat.com>,
	linux-arch@vger.kernel.org, linux-nvdimm@ml01.01.org, hch@lst.de,
	mgorman@suse.de, Matthew Wilcox <willy@linux.intel.com>,
	Ross Zwisler <ross.zwisler@linux.intel.com>,
	riel@redhat.com, david@fromorbit.com, Tejun Heo <tj@kernel.org>,
	axboe@kernel.dk, "Theodore Ts'o" <tytso@mit.edu>,
	"Martin K. Petersen" <martin.petersen@oracle.com>,
	Julia Lawall <Julia.Lawall@lip6.fr>,
	Martin Schwidefsky <schwidefsky@de.ibm.com>,
	linux-fsdevel@vger.kernel.org, akpm@linux-foundation.org,
	Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [PATCH v3 00/11] evacuate struct page from the block layer, introduce __pfn_t
Date: Sat, 23 May 2015 16:32:57 +0200	[thread overview]
Message-ID: <20150523143257.GA19676@lst.de> (raw)
In-Reply-To: <20150512042629.11521.70356.stgit@dwillia2-desk3.amr.corp.intel.com>

I don't like this series at all, it does too much and too little at
the same time.

There's three totally different parts to it that are mixed up:

 (1) cleanups to use accessors for struct scatterlist instead of exposing
     the intricate details of chained S/G list to users
 (2) a switch of struct scatterlist to store a PFN instead of a page
 (3) switch struct bio_vec to store a struct PFN instead of a page

(1) are pretty obvious cleanups, and they should have been submited
separately long time ago.

(2) seems like a good idea, but only when done properly, that is a full
conversion to it.  Not a you need a config option, in which case maybe
some architectures and can sometimes deal with it if they driver says:
meh, okay.

Given that nature of SGLs most consumer want a physical address anyway,
and it shouldn't be a problem to convert all others that need a kernel
mapping to proper helpers.

(3) I'm not sure about in it's current form.  The bio_vec sees all kinds
of highlevel use and we need to be a lot more careful about it, due to
the way we e.g. use the in the iov_iter based read/write interfaces.

It could be that pfn_t based approach there makes sense, but only if
we ensure all consumer can always handle them.

Because of that I'd suggest you try to get (1) and (2) done properly
first, at that point we'll have how we can do (3) without causing 
a big mess.


      parent reply	other threads:[~2015-05-23 14:33 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-05-12  4:29 [PATCH v3 00/11] evacuate struct page from the block layer, introduce __pfn_t Dan Williams
2015-05-12  4:29 ` [PATCH v3 01/11] arch: introduce __pfn_t for persistenti/device memory Dan Williams
2015-05-12  4:29 ` [PATCH v3 02/11] block: add helpers for accessing a bio_vec page Dan Williams
2015-05-12  4:29 ` [PATCH v3 03/11] block: convert .bv_page to .bv_pfn bio_vec Dan Williams
2015-05-12  4:29 ` [PATCH v3 04/11] dma-mapping: allow archs to optionally specify a ->map_pfn() operation Dan Williams
2015-05-12  4:29 ` [PATCH v3 05/11] scatterlist: use sg_phys() Dan Williams
2015-05-12  5:24   ` Julia Lawall
2015-05-12  5:44     ` Dan Williams
2015-05-12  4:30 ` [PATCH v3 06/11] scatterlist: support "page-less" (__pfn_t only) entries Dan Williams
2015-05-13 18:35   ` Williams, Dan J
2015-05-19  4:10     ` Vinod Koul
2015-05-20 16:03       ` Dan Williams
2015-05-23 14:12     ` hch
2015-05-23 16:41       ` Dan Williams
2015-05-12  4:30 ` [PATCH v3 07/11] x86: support dma_map_pfn() Dan Williams
2015-05-12  4:30 ` [PATCH v3 08/11] x86: support kmap_atomic_pfn_t() for persistent memory Dan Williams
2015-05-12  4:30 ` [PATCH v3 09/11] block: convert kmap helpers to kmap_atomic_pfn_t() Dan Williams
2015-05-12  4:30 ` [PATCH v3 10/11] dax: convert to __pfn_t Dan Williams
2015-05-12  4:30 ` [PATCH v3 11/11] block: base support for pfn i/o Dan Williams
2015-05-23 14:32 ` Christoph Hellwig [this message]

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=20150523143257.GA19676@lst.de \
    --to=hch@lst.de \
    --cc=Julia.Lawall@lip6.fr \
    --cc=agk@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=axboe@kernel.dk \
    --cc=benh@kernel.crashing.org \
    --cc=boaz@plexistor.com \
    --cc=clm@fb.com \
    --cc=dan.j.williams@intel.com \
    --cc=dave.hansen@linux.intel.com \
    --cc=david@fromorbit.com \
    --cc=heiko.carstens@de.ibm.com \
    --cc=hpa@zytor.com \
    --cc=j.glisse@gmail.com \
    --cc=jack@suse.cz \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nvdimm@ml01.01.org \
    --cc=martin.petersen@oracle.com \
    --cc=mgorman@suse.de \
    --cc=mingo@kernel.org \
    --cc=neilb@suse.de \
    --cc=paulus@samba.org \
    --cc=riel@redhat.com \
    --cc=ross.zwisler@linux.intel.com \
    --cc=schwidefsky@de.ibm.com \
    --cc=snitzer@redhat.com \
    --cc=tj@kernel.org \
    --cc=torvalds@linux-foundation.org \
    --cc=tytso@mit.edu \
    --cc=willy@linux.intel.com \
    /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).