by David Howells

[permalink] [raw]

Subject: Re: [PATCH 13/14] FS-Cache: Release page->private in failed readahead [try #8]

Andrew Morton <[email protected]> wrote:

> > SetPageLocked(page);
>
> if (TestSetPagLocked(page))
> BUG();
>
> would make me more comfortable..

That shouldn't be necessary if add_to_page_cache() also doesn't do that, but
if you wish, I can do that - it's the error handling path, so it doesn't
matter too much performancewise.

> > > For the second hunk, is it not possible to do this cleanup in the callback
> > > function?
> >
> > Which callback function?
>
> I was referring to the filler_t thingy. Is it not possible to get control
> of that?

Well, the filler_t thing is generally a_ops->readpage from the caller fs, but
we don't want to call that if add_to_page_cache() failed, and we don't want to
call it if we're just discarding a bunch of pages we've now no intention of
actually reading.

I suppose we could add another callback for ditching pages we don't want to
keep. This has the potential to be called quite a lot because of the way
readahead works on Linux.

> So please, can we have some comments in there which describe the new
> behaviour in a manner sufficient for a maintainer to follow so people don't
> break your stuff?

Okay... I'll add more comments. I should probably also extend the
documentation on releasepage(). It won't be till Monday though.

> > Out of interest, why do we need PG_private to say there's something in
> > page->private? Can't it just be assumed either that if page->private is
> > non-zero or that if a_ops->releasepage() is non-NULL, then we need to
> > "release" the page?
>
> page->private is an unsigned long, not a pointer. The core kernel hence
> cannot determine from its value whether or not it is live. For example, the
> fs might choose to treat it as a bitmap of which-blocks-are-uptodate and
> which-blocks-are-dirty.

Then the second option is still possible (calling releasepage()
unconditionally).

David

2006-05-15 05:50:52

by Nathan Scott

[permalink] [raw]

Subject: Re: [PATCH 02/14] NFS: Permit filesystem to perform statfs with a known root dentry [try #9]

Hi David,

On Fri, May 12, 2006 at 11:51:36AM +0100, David Howells wrote:
>
> The attached patch gives the statfs superblock operation a vfsmount pointer
> rather than a superblock pointer.
>
> This complements the get_sb() patch. That reduced the significance of
> sb->s_root, allowing NFS to place a fake root there. However, NFS does require
> a dentry to use as a target for the statfs operation. This permits the root in
> the vfsmount to be used instead.
> ...
> int (*sync_fs)(struct super_block *sb, int wait);
> void (*write_super_lockfs) (struct super_block *);
> void (*unlockfs) (struct super_block *);
> - int (*statfs) (struct super_block *, struct kstatfs *);
> + int (*statfs) (struct vfsmount *, struct kstatfs *);
> int (*remount_fs) (struct super_block *, int *, char *);
> void (*clear_inode) (struct inode *);
> void (*umount_begin) (struct super_block *);

I know this has come up before, and I'm sure it'll come up again
at some point unless we fix it now - could we pass the dentry down
here instead, pretty please?

Miklos asked for it here (and last I spoke to him, he was still
keen on having this available, and his questions on how to do
this cleanly some other way seem to have gone unanswered):
http://www.ussg.iu.edu/hypermail/linux/kernel/0510.3/0234.html

We could now use it in XFS too, hence my interest. We have some
functionality for managing space of a "project" (XFS concept, bit
of a holdover from IRIX thats found new legs recently) within a
filesystem, that provides for dynamically resizable chunks of
space to be carved off of the total filesystem space for these
individual "projects", such that individual projects can be set
space limits (i.e. they are limited to how much space they can
use from the entire filesystem), and we can also report on used
space at a "project" level.

This is done by maintaining project identifiers for every inode in
the tree, with the project identifier being inherited for new nodes
(hardlinks and renames in a project tree are handled as if it were
a separate filesystem, i.e. they're not allowed, EXDEV) and then
using quota by project ID (as opposed to uid/gid) to account and
enforce.

[ Example: we have media customers who want to be able to manage
a single large filesystem on top of a honking big RAID (one fs is
alot easier to backup and administer in general than hundreds...),
but who have multiple groups of artists working on a number of
different films at once.
What they want is to be able to set aside a fixed amount of space
based on an individual films needs, then export that path via NFS,
and later reclaim that space for the next one once the project is
done, but also have the flexibility to easily increase space for a
project when urgently needed <*knock,knock* Ah, Mr Spielberg, how
are you? Yes, sir, right away>. All of this becomes excruciating
when we're forced to work at the volume manager level, of course,
which is where this would otherwise have to be done. ]

This is implemented in XFS and has been merged for awhile now, but
currently the interface for querying free/used space is clunky -
internally XFS is using the quota subsystem to implement this, so
quotactl(2) it is, then there's custom tools, etc. And it is not
exactly a seamless integration with NFS and CIFS, unfortunately,
which we'd really like to provide.

If the VFS passed on the dentry that we have at the statfs syscall
(or from the NFS call, or wherever) we could more cleanly report, in
a way the user expects, on the space used within that project (yes,
I'm aware of the df walking-back-up-to-what-it-thinks-the-root-is
issue, but I also agree with Miklos that its easy to fix).

Anyway, that's why we need it. Miklos gave his sshfs case (see above
link), so it looks like its definately useful in multiple contexts.
And now it sounds like you could use it too (unless there's other
reasons for needing a vfsmount instead of dentry? If I understand the
initial patch description of yours above, it sounds like the dentry
would suffice for you, is that right?). Also, it's not like passing
around a vfsmount is particularly easy, as we can see in this snippet
(and elsewhere) from your patch...

> ...
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> ...
> int
> nfsd_statfs(struct svc_rqst *rqstp, struct svc_fh *fhp, struct kstatfs *stat)
> {
> + struct vfsmount mnt;
> +
> int err = fh_verify(rqstp, fhp, 0, MAY_NOP);
> - if (!err && vfs_statfs(fhp->fh_dentry->d_inode->i_sb,stat))
> - err = nfserr_io;
> + if (!err) {
> + memset(&mnt, 0, sizeof(mnt));
> +
> + mnt.mnt_sb = fhp->fh_dentry->d_inode->i_sb;
> + mnt.mnt_root = mnt.mnt_sb->s_root;
> + mnt.mnt_mountpoint = mnt.mnt_root;
> +
> + if (vfs_statfs(&mnt, stat))
> + err = nfserr_io;
> + }
> return err;

Yuhuuuuck!

So, passing the dentry would actually seem to provide you a cleaner
implementation too. You'd also not need to propogate that mount.h
#include into all those other files which otherwise don't need it.

cheers.

--
Nathan