linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Nick Piggin <npiggin@kernel.dk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Christoph Hellwig <hch@lst.de>,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	stable@kernel.org
Subject: Re: [patch 1/2] fs: fix d_validate (again)
Date: Wed, 15 Dec 2010 12:53:15 +0000	[thread overview]
Message-ID: <20101215125315.GA4419@ZenIV.linux.org.uk> (raw)
In-Reply-To: <20101123102241.GA4317@amd>

On Tue, Nov 23, 2010 at 09:22:41PM +1100, Nick Piggin wrote:
> Again, I'd like to please fix d_validate. It can be trivally fixed
> without delving into filesystem code, and it does not prevent the proper
> careful removal of d_validate and untrusted dentry caches from
> filesystems.
> 
> The right way to merge is fix the bugs in core code now, and remove it
> completely if and when filesystems stop using it. But it's no longer a
> liability or problem to maintain, so it can stay as long as it likes.

TBH, I would prefer to kill the damn thing completely.  Never liked it,
and I think I see how to get rid of it.

Look: what happens is that ncpfs reads the entire directory from server
and since it gets all metadata for free anyway, it creates all dentries
as it goes (assiging i_ino, etc.) and sticks references to them into
page cache for that directory.  readdir() goes through those pages and
as long as all dentries it sees are still alive (and pages still present),
it uses them.  If something's gone or the cache is old enough, we just
evict all those pages and reread.  If some dentries are around during
reread (or initial read, for that matter), we stick references to those
back into the repopulated page cache.

The order of entries matches that in the metadata we'd got from server.

OK.  So let's do that:
	* stick reference back into the page in dentry->d_fsdata (the current
uses are becoming obsolete with new scheme)
	* have d_iput() remove the reference to dentry from page under
spinlock
	* have page eviction remove the references from corresponding dentries
under the same spinlock
	* instead of d_validate() crap, grab the spinlock, lock dentry,
check if it's still alive and do dget_locked() on it if it is (and unlock,
of course).

Voila - d_validate() is no more.  IIRC, smbfs was essentially the same
as ncpfs in that respect.  Comments?

  parent reply	other threads:[~2010-12-15 12:53 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-11-23 10:22 [patch 1/2] fs: fix d_validate (again) Nick Piggin
2010-11-23 10:26 ` [patch 2/2] mm: remove (kmem|kern)_ptr_validate Nick Piggin
2010-12-15 12:53 ` Al Viro [this message]
2010-12-16  2:46   ` [patch 1/2] fs: fix d_validate (again) Nick Piggin

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=20101215125315.GA4419@ZenIV.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --cc=hch@lst.de \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=npiggin@kernel.dk \
    --cc=stable@kernel.org \
    --cc=torvalds@linux-foundation.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).