Message-ID: <alpine.LFD.2.00.0912170928310.15740@localhost.localdomain>
Date:	Thu, 17 Dec 2009 18:57:02 +0100
From:	James Bottomley <james.bottomley@...e.de>
To:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Christoph Hellwig <hch@...radead.org>
Cc:	tytso@....edu, Kyle McMartin <kyle@...artin.ca>,
	linux-parisc@...r.kernel.org,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	James.Bottomley@...e.de, linux-arch@...r.kernel.org,
	Jens Axboe <jens.axboe@...cle.com>
Subject: Re: [git patches] xfs and block fixes for virtually indexed arches

Linus Torvalds <torvalds@...ux-foundation.org> wrote:

>
>
>On Thu, 17 Dec 2009, Christoph Hellwig wrote:
>>
>> On Thu, Dec 17, 2009 at 08:46:33AM -0800, Linus Torvalds wrote:
>> > The whole "vmalloc is special" has always been true. If you want to 
>> > treat vmalloc as normal memory, you need to look up the pages yourself. We 
>> > have helpers for that (including helpers that populate vmalloc space from 
>> > a page array to begin with - so you can _start_ from some array of pages 
>> > and then lay them out virtually if you want to have a convenient CPU 
>> > access to the array).
>> 
>> Which is exactly what the XFS code does.  Pages are allocated manually
>> and we store pointers to the page struct that later get added to the
>> bio.
>
>Hmm. The BIO interface that the patch-series changes (bio_map_kern) 
>doesn't work that way. It takes a "buf, len" kind of thing. That's what 
>I'm complaining about.
>
>> But we access them through vmap (which I added exactly for this
>> reason back in 2002) for kernel accesses.  On all architectures with
>> sane caches things just work, but for parisc, arm and friends that have
>> virtually indexed caches we need to make sure to flush caches for this
>> different access.  The vmalloc linear address is not used for I/O
>> everywhere.
>
>Well, they clearly are _after_ this series, since that's what all those 
>changes to __bio_map_kernel() and bio_map_kern_endio() are all about.
>
>So I believe you when you say that XFS perhaps does everything right - I 
>just think that the patch series in question actually makes things worse, 
>exactly because it is starting to use virtual addresses.
>
>I also think that the changes to bio_map_kernel() and bio_map_kern_endio() 
>are not just "fundamentally ugly", I think they are made worse by the fact 
>that it's not even done "right". You both flush the virtual caches before 
>the IO and invalidate after - when the real pattern should be that you 
>flush it before a write, and invalidate it after a read.

Yes, will fix.

>And I really think that would be all much more properly done at the 
>_caller_ level, not by the BIO layer.

Agree on this, will redo the patch.

>You must have some locking and allocation etc logic at the caller anyway, 
>why doesn't _that_ level just do the flushing or invalidation?
>
>I get the feeling that somebody decided that the whole "do DMA to/from 
>vmalloc space" was somehow a common generic pattern that should be 
>supported in general, and I violently disagree. Maybe XFS has good reasons 
>for doing it, but that does emphatically _not_ make it a good idea in 
>general, and that does _not_ mean that the BIO layer should make it easy 
>to do for other users and have a general interface for that kind of 
>crazyness.
>
>IOW, I'm perfectly happy with the patch to fs/xfs/linux-2.6/xfs_buf.c. 
>That one still seems to use 'bio_add_page()' with a regular 'struct page'. 
>
>But the fs/bio.c patch looks like just total and utter crap to me, and is 
>the reason I refuse to pull this series.

OK, will fix and resend.

James

Powered by blists - more mailing lists