[PATCH -mm] unionfs: build fixes

[PATCH -mm] unionfs: build fixes

[Hugh Dickins]

Get unionfs building and working in mmotm with the 2.6.27-rc1 VFS changes:
permission() has been replaced by inode_permission() without nameidata arg;
unionfs_permission() without nameidata arg; vfs_symlink() without mode arg;
LOOKUP_ACCESS no longer defined; and kmem_cache_create() no longer passes
kmem_cachep to the init_once() constructor.

Note: while okay for inclusion in -mm for now, unionfs_permission() mods
will need review and perhaps correction by Erez: without a nameidata arg,
some locking vanishes from unionfs_permission(), and a MNT_NOEXEC check on
its lower_inode; I have not studied the VFS changes enough to tell whether
that amounts to a real issue for unionfs, or just removal of dead code.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
Cc: Erez Zadok <ezk@cs.sunysb.edu>
---
This should follow git-unionfs.patch
I notice my unionfs-fix-memory-leak.patch
and fsstack-fsstack_copy_inode_size-locking.patch
are currently commented out, yet I don't recall the
mm-commits dispatch rider bringing me a telegram to explain why?

 fs/unionfs/dirhelper.c |    2 +-
 fs/unionfs/inode.c     |   16 +++-------------
 fs/unionfs/lookup.c    |    5 +----
 fs/unionfs/sioq.c      |    2 +-
 fs/unionfs/super.c     |    2 +-
 5 files changed, 7 insertions(+), 20 deletions(-)

--- mmotm.orig/fs/unionfs/dirhelper.c	2008-07-29 17:47:09.000000000 +0100
+++ mmotm/fs/unionfs/dirhelper.c	2008-07-29 18:26:12.000000000 +0100
@@ -110,7 +110,7 @@ int delete_whiteouts(struct dentry *dent
 	lower_dir = lower_dir_dentry->d_inode;
 	BUG_ON(!S_ISDIR(lower_dir->i_mode));
 
-	if (!permission(lower_dir, MAY_WRITE | MAY_EXEC, NULL)) {
+	if (!inode_permission(lower_dir, MAY_WRITE | MAY_EXEC)) {
 		err = do_delete_whiteouts(dentry, bindex, namelist);
 	} else {
 		args.deletewh.namelist = namelist;
--- mmotm.orig/fs/unionfs/inode.c	2008-07-29 18:21:46.000000000 +0100
+++ mmotm/fs/unionfs/inode.c	2008-07-29 18:26:12.000000000 +0100
@@ -462,7 +462,6 @@ static int unionfs_symlink(struct inode 
 	struct dentry *lower_parent_dentry = NULL;
 	char *name = NULL;
 	int valid = 0;
-	umode_t mode;
 
 	unionfs_read_lock(dentry->d_sb, UNIONFS_SMUTEX_CHILD);
 	unionfs_lock_dentry(dentry, UNIONFS_DMUTEX_CHILD);
@@ -497,9 +496,7 @@ static int unionfs_symlink(struct inode 
 		goto out;
 	}
 
-	mode = S_IALLUGO;
-	err = vfs_symlink(lower_parent_dentry->d_inode, lower_dentry,
-			  symname, mode);
+	err = vfs_symlink(lower_parent_dentry->d_inode, lower_dentry, symname);
 	if (!err) {
 		err = PTR_ERR(unionfs_interpose(dentry, parent->i_sb, 0));
 		if (!err) {
@@ -872,8 +869,7 @@ static void unionfs_put_link(struct dent
  * unionfs_permission, or anything it calls, will use stale branch
  * information.
  */
-static int unionfs_permission(struct inode *inode, int mask,
-			      struct nameidata *nd)
+static int unionfs_permission(struct inode *inode, int mask)
 {
 	struct inode *lower_inode = NULL;
 	int err = 0;
@@ -881,9 +877,6 @@ static int unionfs_permission(struct ino
 	const int is_file = !S_ISDIR(inode->i_mode);
 	const int write_mask = (mask & MAY_WRITE) && !(mask & MAY_READ);
 
-	if (nd)
-		unionfs_lock_dentry(nd->path.dentry, UNIONFS_DMUTEX_CHILD);
-
 	if (!UNIONFS_I(inode)->lower_inodes) {
 		if (is_file)	/* dirs can be unlinked but chdir'ed to */
 			err = -ESTALE;	/* force revalidate */
@@ -923,7 +916,7 @@ static int unionfs_permission(struct ino
 		 * readonly, because those conditions should lead to a
 		 * copyup taking place later on.
 		 */
-		err = permission(lower_inode, mask, nd);
+		err = inode_permission(lower_inode, mask);
 		if (err && bindex > 0) {
 			umode_t mode = lower_inode->i_mode;
 			if (is_robranch_super(inode->i_sb, bindex) &&
@@ -955,9 +948,6 @@ static int unionfs_permission(struct ino
 
 out:
 	unionfs_check_inode(inode);
-	unionfs_check_nd(nd);
-	if (nd)
-		unionfs_unlock_dentry(nd->path.dentry);
 	return err;
 }
 
--- mmotm.orig/fs/unionfs/lookup.c	2008-07-29 18:21:46.000000000 +0100
+++ mmotm/fs/unionfs/lookup.c	2008-07-29 18:26:12.000000000 +0100
@@ -47,7 +47,7 @@ static noinline_for_stack int is_opaque_
 
 	mutex_lock(&lower_inode->i_mutex);
 
-	if (!permission(lower_inode, MAY_EXEC, NULL)) {
+	if (!inode_permission(lower_inode, MAY_EXEC)) {
 		wh_lower_dentry =
 			lookup_one_len(UNIONFS_DIR_OPAQUE, lower_dentry,
 				       sizeof(UNIONFS_DIR_OPAQUE) - 1);
@@ -635,9 +635,6 @@ int init_lower_nd(struct nameidata *nd, 
 		nd->intent.open.file = file;
 #endif /* ALLOC_LOWER_ND_FILE */
 		break;
-	case LOOKUP_ACCESS:
-		nd->flags = flags;
-		break;
 	default:
 		/*
 		 * We should never get here, for now.
--- mmotm.orig/fs/unionfs/sioq.c	2008-07-29 18:21:46.000000000 +0100
+++ mmotm/fs/unionfs/sioq.c	2008-07-29 18:26:12.000000000 +0100
@@ -87,7 +87,7 @@ void __unionfs_symlink(struct work_struc
 	struct sioq_args *args = container_of(work, struct sioq_args, work);
 	struct symlink_args *s = &args->symlink;
 
-	args->err = vfs_symlink(s->parent, s->dentry, s->symbuf, s->mode);
+	args->err = vfs_symlink(s->parent, s->dentry, s->symbuf);
 	complete(&args->comp);
 }
 
--- mmotm.orig/fs/unionfs/super.c	2008-07-29 18:21:46.000000000 +0100
+++ mmotm/fs/unionfs/super.c	2008-07-29 18:26:12.000000000 +0100
@@ -904,7 +904,7 @@ static void unionfs_destroy_inode(struct
 }
 
 /* unionfs inode cache constructor */
-static void init_once(struct kmem_cache *cachep, void *obj)
+static void init_once(void *obj)
 {
 	struct unionfs_inode_info *i = obj;
 

Re: [PATCH -mm] unionfs: build fixes

[Andrew Morton]

On Tue, 29 Jul 2008 19:12:47 +0100 (BST)
Hugh Dickins <hugh@veritas.com> wrote:

> Get unionfs building and working in mmotm with the 2.6.27-rc1 VFS changes:
> permission() has been replaced by inode_permission() without nameidata arg;
> unionfs_permission() without nameidata arg; vfs_symlink() without mode arg;
> LOOKUP_ACCESS no longer defined; and kmem_cache_create() no longer passes
> kmem_cachep to the init_once() constructor.
> 
> Note: while okay for inclusion in -mm for now, unionfs_permission() mods
> will need review and perhaps correction by Erez: without a nameidata arg,
> some locking vanishes from unionfs_permission(), and a MNT_NOEXEC check on
> its lower_inode; I have not studied the VFS changes enough to tell whether
> that amounts to a real issue for unionfs, or just removal of dead code.

thanks.

> This should follow git-unionfs.patch
> I notice my unionfs-fix-memory-leak.patch
> and fsstack-fsstack_copy_inode_size-locking.patch
> are currently commented out, yet I don't recall the
> mm-commits dispatch rider bringing me a telegram to explain why?

git-unionfs got commented out because of some upstream git (or build)
catastrophe.  So its fixes got comemnted out too.  Then git-unionfs was
restored but I forgot to manually restore the followon fixes.  It
happens.

I must say that I'm not really sure why we're struggling along with
unionfs.  Last I heard there were fundamental, unresolveable design
disagreements with the VFS guys.  Those issues should be where 100% of
the effort is being devoted, but instead we seem to be cruising along
in a different direction?

Re: [Unionfs] Re: [PATCH -mm] unionfs: build fixes

[Erez Zadok]

In message <20080729172216.84e958f7.akpm@linux-foundation.org>, Andrew Morton writes:
> On Tue, 29 Jul 2008 19:12:47 +0100 (BST)
> Hugh Dickins <hugh@veritas.com> wrote:
> 
> > Get unionfs building and working in mmotm with the 2.6.27-rc1 VFS changes:
> > permission() has been replaced by inode_permission() without nameidata arg;
> > unionfs_permission() without nameidata arg; vfs_symlink() without mode arg;
> > LOOKUP_ACCESS no longer defined; and kmem_cache_create() no longer passes
> > kmem_cachep to the init_once() constructor.
> > 
> > Note: while okay for inclusion in -mm for now, unionfs_permission() mods
> > will need review and perhaps correction by Erez: without a nameidata arg,
> > some locking vanishes from unionfs_permission(), and a MNT_NOEXEC check on
> > its lower_inode; I have not studied the VFS changes enough to tell whether
> > that amounts to a real issue for unionfs, or just removal of dead code.
> 
> thanks.
> 
> > This should follow git-unionfs.patch
> > I notice my unionfs-fix-memory-leak.patch
> > and fsstack-fsstack_copy_inode_size-locking.patch
> > are currently commented out, yet I don't recall the
> > mm-commits dispatch rider bringing me a telegram to explain why?
> 
> git-unionfs got commented out because of some upstream git (or build)
> catastrophe.  So its fixes got comemnted out too.  Then git-unionfs was
> restored but I forgot to manually restore the followon fixes.  It
> happens.

Shortly I'm going to post fixes which include Hugh's stuff and more.  Sorry
for the delay.

> I must say that I'm not really sure why we're struggling along with
> unionfs.  Last I heard there were fundamental, unresolveable design
> disagreements with the VFS guys.  Those issues should be where 100% of
> the effort is being devoted, but instead we seem to be cruising along
> in a different direction?

Some of my upcoming patches begin to address this (took longer than
expected):

- extracting all whiteout related code into callable methods in unionfs, so
  that I can "drop in" the new whiteout code that Bharata et al. are
  reportedly working on.  I really hope to see some new whiteout code in -mm
  soon.  Bharata?

- reworking the lookup code to handle vfsmounts: this'll be needed when we
  switch from vfs_* to path_* (Miklos's patches).

As for other fundamental issues, I've been posting some suggestions in
recent months.  For example

- the need for cleaner handling of vma->fault(), a relatively minor patch I
  posted, based on hch's LSF08 suggestions.  Got no response from any of the
  VFS folks.

- a post I made regarding suggestions on how to handle lower f/s changes,
  based on Viro's LSF08 comments: to have a superblock level writers count
  (I suggested that it's a superset of the superblock->s_vfs_rename_mutex,
  and perhaps be elevated to be one).  Again, got no responses from anyone
  on the VFS team.

So I'm not sure how much the VFS guys have time now to review such patches
and help me address these issues.  We can't seem to get through even simpler
issues, nor get simple patches merged (ala the copy_inode_size) despite
repeated attempts.

Suggestions how proceed next are very welcome.

Erez.

Re: [Unionfs] Re: [PATCH -mm] unionfs: build fixes

[Bharata B Rao]

On Tue, Jul 29, 2008 at 09:02:03PM -0400, Erez Zadok wrote:
> In message <20080729172216.84e958f7.akpm@linux-foundation.org>, Andrew Morton writes:
> > On Tue, 29 Jul 2008 19:12:47 +0100 (BST)
> > Hugh Dickins <hugh@veritas.com> wrote:
> > 
> > > Get unionfs building and working in mmotm with the 2.6.27-rc1 VFS changes:
> > > permission() has been replaced by inode_permission() without nameidata arg;
> > > unionfs_permission() without nameidata arg; vfs_symlink() without mode arg;
> > > LOOKUP_ACCESS no longer defined; and kmem_cache_create() no longer passes
> > > kmem_cachep to the init_once() constructor.
> > > 
> > > Note: while okay for inclusion in -mm for now, unionfs_permission() mods
> > > will need review and perhaps correction by Erez: without a nameidata arg,
> > > some locking vanishes from unionfs_permission(), and a MNT_NOEXEC check on
> > > its lower_inode; I have not studied the VFS changes enough to tell whether
> > > that amounts to a real issue for unionfs, or just removal of dead code.
> > 
> > thanks.
> > 
> > > This should follow git-unionfs.patch
> > > I notice my unionfs-fix-memory-leak.patch
> > > and fsstack-fsstack_copy_inode_size-locking.patch
> > > are currently commented out, yet I don't recall the
> > > mm-commits dispatch rider bringing me a telegram to explain why?
> > 
> > git-unionfs got commented out because of some upstream git (or build)
> > catastrophe.  So its fixes got comemnted out too.  Then git-unionfs was
> > restored but I forgot to manually restore the followon fixes.  It
> > happens.
> 
> Shortly I'm going to post fixes which include Hugh's stuff and more.  Sorry
> for the delay.
> 
> > I must say that I'm not really sure why we're struggling along with
> > unionfs.  Last I heard there were fundamental, unresolveable design
> > disagreements with the VFS guys.  Those issues should be where 100% of
> > the effort is being devoted, but instead we seem to be cruising along
> > in a different direction?
> 
> Some of my upcoming patches begin to address this (took longer than
> expected):
> 
> - extracting all whiteout related code into callable methods in unionfs, so
>   that I can "drop in" the new whiteout code that Bharata et al. are
>   reportedly working on.  I really hope to see some new whiteout code in -mm
>   soon.  Bharata?

When I last checked, David Woodhouse/Jan Blunck were working on this.
Looks like they have become busy with other work now. I can take that work
forward.

Jan/David, if this means duplicating your efforts, please let me know.

Regards,
Bharata.

Re: [Unionfs] Re: [PATCH -mm] unionfs: build fixes

[Erez Zadok]

In message <20080730032738.GA3977@in.ibm.com>, Bharata B Rao writes:
> On Tue, Jul 29, 2008 at 09:02:03PM -0400, Erez Zadok wrote:
[...]

> When I last checked, David Woodhouse/Jan Blunck were working on this.
> Looks like they have become busy with other work now. I can take that work
> forward.

Now that I've restructured and refactored the whiteout code in unionfs, I
can easily be your Guinea pig. :-)

Thanks,
Erez.

Re: [Unionfs] Re: [PATCH -mm] unionfs: build fixes

[Ian Kent]

On Tue, 2008-07-29 at 21:02 -0400, Erez Zadok wrote:
> In message <20080729172216.84e958f7.akpm@linux-foundation.org>, Andrew Morton writes:
> > On Tue, 29 Jul 2008 19:12:47 +0100 (BST)
> > Hugh Dickins <hugh@veritas.com> wrote:
> > 
> > > Get unionfs building and working in mmotm with the 2.6.27-rc1 VFS changes:
> > > permission() has been replaced by inode_permission() without nameidata arg;
> > > unionfs_permission() without nameidata arg; vfs_symlink() without mode arg;
> > > LOOKUP_ACCESS no longer defined; and kmem_cache_create() no longer passes
> > > kmem_cachep to the init_once() constructor.
> > > 
> > > Note: while okay for inclusion in -mm for now, unionfs_permission() mods
> > > will need review and perhaps correction by Erez: without a nameidata arg,
> > > some locking vanishes from unionfs_permission(), and a MNT_NOEXEC check on
> > > its lower_inode; I have not studied the VFS changes enough to tell whether
> > > that amounts to a real issue for unionfs, or just removal of dead code.
> > 
> > thanks.
> > 
> > > This should follow git-unionfs.patch
> > > I notice my unionfs-fix-memory-leak.patch
> > > and fsstack-fsstack_copy_inode_size-locking.patch
> > > are currently commented out, yet I don't recall the
> > > mm-commits dispatch rider bringing me a telegram to explain why?
> > 
> > git-unionfs got commented out because of some upstream git (or build)
> > catastrophe.  So its fixes got comemnted out too.  Then git-unionfs was
> > restored but I forgot to manually restore the followon fixes.  It
> > happens.
> 
> Shortly I'm going to post fixes which include Hugh's stuff and more.  Sorry
> for the delay.
> 
> > I must say that I'm not really sure why we're struggling along with
> > unionfs.  Last I heard there were fundamental, unresolveable design
> > disagreements with the VFS guys.  Those issues should be where 100% of
> > the effort is being devoted, but instead we seem to be cruising along
> > in a different direction?
> 
> Some of my upcoming patches begin to address this (took longer than
> expected):
> 
> - extracting all whiteout related code into callable methods in unionfs, so
>   that I can "drop in" the new whiteout code that Bharata et al. are
>   reportedly working on.  I really hope to see some new whiteout code in -mm
>   soon.  Bharata?
> 
> - reworking the lookup code to handle vfsmounts: this'll be needed when we
>   switch from vfs_* to path_* (Miklos's patches).
> 
> As for other fundamental issues, I've been posting some suggestions in
> recent months.  For example
> 
> - the need for cleaner handling of vma->fault(), a relatively minor patch I
>   posted, based on hch's LSF08 suggestions.  Got no response from any of the
>   VFS folks.
> 
> - a post I made regarding suggestions on how to handle lower f/s changes,
>   based on Viro's LSF08 comments: to have a superblock level writers count
>   (I suggested that it's a superset of the superblock->s_vfs_rename_mutex,
>   and perhaps be elevated to be one).  Again, got no responses from anyone
>   on the VFS team.

Erez, do you have links to email threads or a commentary of the things
that are causing concern somewhere?

I spotted one but it seems light on for descriptive value (or maybe it's
me who's light on for understanding, ;)).

> 
> So I'm not sure how much the VFS guys have time now to review such patches
> and help me address these issues.  We can't seem to get through even simpler
> issues, nor get simple patches merged (ala the copy_inode_size) despite
> repeated attempts.

Yeah, life is like that a lot for me too.

But why not assume that, given a reasonable amount of time, a "no
response" is equivalent to a "no complaints" and push on with the
updates. Sooner or later someone who cares enough will take a look and
give the needed feedback.

Ian

Re: [PATCH -mm] unionfs: build fixes

[Erez Zadok]

In message <Pine.LNX.4.64.0807291907440.13365@blonde.site>, Hugh Dickins writes:
> Get unionfs building and working in mmotm with the 2.6.27-rc1 VFS changes:
> permission() has been replaced by inode_permission() without nameidata arg;
> unionfs_permission() without nameidata arg; vfs_symlink() without mode arg;
> LOOKUP_ACCESS no longer defined; and kmem_cache_create() no longer passes
> kmem_cachep to the init_once() constructor.
> 
> Note: while okay for inclusion in -mm for now, unionfs_permission() mods
> will need review and perhaps correction by Erez: without a nameidata arg,
> some locking vanishes from unionfs_permission(), and a MNT_NOEXEC check on
> its lower_inode; I have not studied the VFS changes enough to tell whether
> that amounts to a real issue for unionfs, or just removal of dead code.

Thanks Hugh.  You beat me by few hours. :-) I've been testing similar fixes
for several days already (and was waiting for the dust to settle in the
merge window, b/c patches flying b/t linus's tree, -next, and -mm).

I've also looked at the inode_permission() locking, and tested it, and I
think it should be safe as long as the refcnt on the inode doesn't go to
zero.  In fact, I think unionfs_permission can be possibly further trimmed.

> Signed-off-by: Hugh Dickins <hugh@veritas.com>
> Cc: Erez Zadok <ezk@cs.sunysb.edu>
> ---
> This should follow git-unionfs.patch
> I notice my unionfs-fix-memory-leak.patch
> and fsstack-fsstack_copy_inode_size-locking.patch
> are currently commented out, yet I don't recall the
> mm-commits dispatch rider bringing me a telegram to explain why?

Both of those patches are in my tree now (not yet pushed to korg).

Note: your fsstack-fsstack_copy_inode_size-locking patch should really go to
mainline, but the one you sent depends on code in unionfs.  So temporarily
I'm folding it into my tree.  In a couple of days (once I switch from using
git to something like quilt) I'll extract it out into a separate patch which
can be applied cleanly against linux-next, so that Andrew can hopefully send
it upstream.

Erez.