mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Hugh Dickins <hugh@veritas.com>
To: Arjan van de Ven <arjan@infradead.org>
Cc: Josef Sipek <jsipek@fsl.cs.sunysb.edu>, linux-kernel@vger.kernel.org
Subject: Re: swapper_space export
Date: Wed, 17 May 2006 16:52:36 +0100 (BST)	[thread overview]
Message-ID: <Pine.LNX.4.64.0605171626460.6711@blonde.wat.veritas.com> (raw)
In-Reply-To: <1147864721.3051.17.camel@laptopd505.fenrus.org>

On Wed, 17 May 2006, Arjan van de Ven wrote:
> On Tue, 2006-05-16 at 19:24 -0400, Josef Sipek wrote:
> > I was trying to compile the Unionfs[1] to get it up to sync it up with
> > the kernel developments from the past few months. Anyway, long story
> > short...swapper_space (defined in mm/swap_state.c) is not exported
> > anymore (git commit: 4936967374c1ad0eb3b734f24875e2484c3786cc). This
> > apparently is not an issue for most modules. Troubles arise when the
> > modules include mm.h or any of its relatives.
> > 
> > One simply gets a linker error about swapper_space not being defined.
> > The problem is that it is used in mm.h.
> 
> don't you think it's really suspect that no other filesystem, in fact no
> other driver in the kernel needs this? Could it just be that unionfs is 
> using a wrong API ? Because if that's the case you're patch is just the
> wrong thing. Maybe the unionfs people should try to submit their code
> for review etc......

Much as I'd love to side with Jeff against you and Adrian ;)
I think you're right.

I see no reference to page_mapping() in the unionfs source (and at
present there's no other justifiable modular use of swapper_space);
but my guess would be that Jeff is being more conscientious than is
called for in getting it to sync up with the kernel -

The unionfs source does contain its own inline "sync_page" which
comments that it "is copied verbatim from mm/filemap.c".  I'm
guessing Jeff has noticed that it's no longer a verbatim copy,
has made it so, and is thereby involving page_mapping().

No need for that here (nor for the smp_mb nor for the io_schedule):
unionfs's sync_page is working on a locked pagecache page of the
lower-level filesystem, that's not going to be a PageSwapCache page
nor a PageAnon page (nor even a truncated page with NULL mapping:
page lock is held).  Just use page->mapping as before.

(But I notice that unionfs better not have a tmpfs in its union:
the unionfs use of grab_cache_page is not strictly compatible with
the way tmpfs pages are swapped out under memory pressure.)

Hugh

  parent reply	other threads:[~2006-05-17 15:52 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2006-05-16 23:24 Josef Sipek
2006-05-17 10:15 ` Adrian Bunk
2006-05-17 11:18 ` Arjan van de Ven
2006-05-17 15:22   ` Andrew Morton
2006-05-17 15:52   ` Hugh Dickins [this message]
2006-05-17 19:12     ` Josef Sipek

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=Pine.LNX.4.64.0605171626460.6711@blonde.wat.veritas.com \
    --to=hugh@veritas.com \
    --cc=arjan@infradead.org \
    --cc=jsipek@fsl.cs.sunysb.edu \
    --cc=linux-kernel@vger.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

all inboxes | Powered by JetHome®