linux-next: ubifs tree build failure

linux-next: ubifs tree build failure

[Stephen Rothwell]

Hi Artem,

Today's linux-next build (x86_64 allmodconfig) failed like this:

fs/ubifs/super.c: In function 'ubifs_fh_to_dentry':
fs/ubifs/super.c:1611: error: implicit declaration of function 'd_alloc_anon'
fs/ubifs/super.c:1611: warning: assignment makes pointer from integer without a cast

Caused by interaction between commit
95b53e84e8899258a1af011a7216abb978afae26 ("kill d_alloc_anon") from the
vfs tree and b51e4badb0248ac75fe3128bb369d987e28b2f88 ("UBIFS: add NFS
support") from the ubifs tree.  The former removes d_alloc_anon and the
latter adds a new usage.

For now, I have reverted the ubifs commit.  Christoph, maybe you could
provide a better solution.

I have appended the reverted patch below.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

commit b51e4badb0248ac75fe3128bb369d987e28b2f88
Author: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Date:   Tue Jul 22 16:49:31 2008 +0300

    UBIFS: add NFS support
    
    Implement minimal set of FS export operations to support NFS.
    
    Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index a79e850..320f89a 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -255,12 +255,7 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry,
 
 done:
 	kfree(dent);
-	/*
-	 * Note, d_splice_alias() would be required instead if we supported
-	 * NFS.
-	 */
-	d_add(dentry, inode);
-	return NULL;
+	return d_splice_alias(inode, dentry);
 
 out:
 	kfree(dent);
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 1dc8c25..39a6e01 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -35,6 +35,7 @@
 #include <linux/parser.h>
 #include <linux/seq_file.h>
 #include <linux/mount.h>
+#include <linux/exportfs.h>
 #include "ubifs.h"
 
 /* Slab cache for UBIFS inodes */
@@ -149,7 +150,7 @@ struct inode *ubifs_iget(struct super_block *sb, unsigned long inum)
 	if (err)
 		goto out_invalid;
 
-	/* Disable readahead */
+	/* Disable read-ahead */
 	inode->i_mapping->backing_dev_info = &c->bdi;
 
 	switch (inode->i_mode & S_IFMT) {
@@ -345,7 +346,7 @@ static void ubifs_delete_inode(struct inode *inode)
 	if (err)
 		/*
 		 * Worst case we have a lost orphan inode wasting space, so a
-		 * simple error message is ok here.
+		 * simple error message is OK here.
 		 */
 		ubifs_err("can't delete inode %lu, error %d",
 			  inode->i_ino, err);
@@ -1578,6 +1579,51 @@ struct super_operations ubifs_super_operations = {
 	.sync_fs       = ubifs_sync_fs,
 };
 
+/*
+ * Note, since UBIFS does re-use inode numbers at the moment, we do not check
+ * the generation number in this function.
+ */
+static struct dentry *ubifs_fh_to_dentry(struct super_block *sb,
+					 struct fid *fid,
+					 int fh_len, int fh_type)
+{
+	ino_t inum;
+	struct inode *inode;
+	struct dentry *dent;
+
+	switch (fh_type) {
+	case FILEID_INO32_GEN:
+	case FILEID_INO32_GEN_PARENT:
+		inum = fid->i32.ino;
+		break;
+	default:
+		dbg_err("unsupported file handle type %d", fh_type);
+		return ERR_PTR(-EINVAL);
+	}
+
+	inode = ubifs_iget(sb, inum);
+	if (IS_ERR(inode)) {
+		if (PTR_ERR(inode) == -ENOENT)
+			return ERR_PTR(-ESTALE);
+		return ERR_CAST(inode);
+	}
+
+	dent = d_alloc_anon(inode);
+	if (!dent) {
+		iput(inode);
+		return ERR_PTR(-ENOMEM);
+	}
+	return dent;
+}
+
+/*
+ * Probably NFS support could be faster if other export operations were
+ * implemented, but '->fh_to_dentry()' is enough.
+ */
+static struct export_operations ubifs_export_ops = {
+	.fh_to_dentry = ubifs_fh_to_dentry,
+};
+
 /**
  * open_ubi - parse UBI device name string and open the UBI device.
  * @name: UBI volume name
@@ -1685,10 +1731,10 @@ static int ubifs_fill_super(struct super_block *sb, void *data, int silent)
 	}
 
 	/*
-	 * UBIFS provids 'backing_dev_info' in order to disable readahead. For
+	 * UBIFS provides 'backing_dev_info' in order to disable read-ahead. For
 	 * UBIFS, I/O is not deferred, it is done immediately in readpage,
 	 * which means the user would have to wait not just for their own I/O
-	 * but the readahead I/O as well i.e. completely pointless.
+	 * but the read-ahead I/O as well i.e. completely pointless.
 	 *
 	 * Read-ahead will be disabled because @c->bdi.ra_pages is 0.
 	 */
@@ -1713,6 +1759,7 @@ static int ubifs_fill_super(struct super_block *sb, void *data, int silent)
 	if (c->max_inode_sz > MAX_LFS_FILESIZE)
 		sb->s_maxbytes = c->max_inode_sz = MAX_LFS_FILESIZE;
 	sb->s_op = &ubifs_super_operations;
+	sb->s_export_op = &ubifs_export_ops;
 
 	mutex_lock(&c->umount_mutex);
 	err = mount_ubifs(c);
@@ -1846,10 +1893,11 @@ static void ubifs_kill_sb(struct super_block *sb)
 }
 
 static struct file_system_type ubifs_fs_type = {
-	.name    = "ubifs",
-	.owner   = THIS_MODULE,
-	.get_sb  = ubifs_get_sb,
-	.kill_sb = ubifs_kill_sb
+	.name     = "ubifs",
+	.owner    = THIS_MODULE,
+	.get_sb   = ubifs_get_sb,
+	.kill_sb  = ubifs_kill_sb,
+	.fs_flags = FS_REQUIRES_DEV,
 };
 
 /*

Re: linux-next: ubifs tree build failure

[Artem Bityutskiy]

Stephen Rothwell wrote:
> Today's linux-next build (x86_64 allmodconfig) failed like this:
> 
> fs/ubifs/super.c: In function 'ubifs_fh_to_dentry':
> fs/ubifs/super.c:1611: error: implicit declaration of function 'd_alloc_anon'
> fs/ubifs/super.c:1611: warning: assignment makes pointer from integer without a cast

Hi Stephen,

I'll look at this, thanks.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: linux-next: ubifs tree build failure

[Artem Bityutskiy]

ext Stephen Rothwell wrote:
> Hi Artem,
> 
> Today's linux-next build (x86_64 allmodconfig) failed like this:
> 
> fs/ubifs/super.c: In function 'ubifs_fh_to_dentry':
> fs/ubifs/super.c:1611: error: implicit declaration of function 'd_alloc_anon'
> fs/ubifs/super.c:1611: warning: assignment makes pointer from integer without a cast
> 
> Caused by interaction between commit
> 95b53e84e8899258a1af011a7216abb978afae26 ("kill d_alloc_anon") from the
> vfs tree and b51e4badb0248ac75fe3128bb369d987e28b2f88 ("UBIFS: add NFS
> support") from the ubifs tree.  The former removes d_alloc_anon and the
> latter adds a new usage.
> 
> For now, I have reverted the ubifs commit.  Christoph, maybe you could
> provide a better solution.
> 
> I have appended the reverted patch below.

I am not sure how to resolve things like this actually. I am planning to
send ubifs-2.6.git upstream soon, so probably Christoph will be able to amend
his stuff after this. Or any other suggestion?

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: linux-next: ubifs tree build failure

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 717 bytes --]

Hi Artem,

On Tue, 12 Aug 2008 10:37:28 +0300 Artem Bityutskiy <Artem.Bityutskiy@nokia.com> wrote:
>
> I am not sure how to resolve things like this actually. I am planning to
> send ubifs-2.6.git upstream soon, so probably Christoph will be able to amend
> his stuff after this. Or any other suggestion?

That is the thing to do (assuming that Linus will take your tree) as it
will push the pain back onto the one making the API change ... and (as
you say) Christoph will be able to fix it up in his patch set.

/me wonders how many more of these we will get before the next merge window opens

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: linux-next: ubifs tree build failure

[Christoph Hellwig]

On Tue, Aug 12, 2008 at 04:24:09PM +1000, Stephen Rothwell wrote:
> For now, I have reverted the ubifs commit.  Christoph, maybe you could
> provide a better solution.

Artem, could you _please_ send ubifs patches to fsdevel for review
before putting them into -next?  Even more so for nfs exporting stuff
as it needs quite a bit of review.

> @@ -149,7 +150,7 @@ struct inode *ubifs_iget(struct super_block *sb, unsigned long inum)
>  	if (err)
>  		goto out_invalid;
>  
> -	/* Disable readahead */
> +	/* Disable read-ahead */
>  	inode->i_mapping->backing_dev_info = &c->bdi;
>  
>  	switch (inode->i_mode & S_IFMT) {
> @@ -345,7 +346,7 @@ static void ubifs_delete_inode(struct inode *inode)
>  	if (err)
>  		/*
>  		 * Worst case we have a lost orphan inode wasting space, so a
> -		 * simple error message is ok here.
> +		 * simple error message is OK here.
>  		 */

What are these comment changes doing in a patch adding export ops?

> +	switch (fh_type) {
> +	case FILEID_INO32_GEN:
> +	case FILEID_INO32_GEN_PARENT:
> +		inum = fid->i32.ino;
> +		break;
> +	default:
> +		dbg_err("unsupported file handle type %d", fh_type);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	inode = ubifs_iget(sb, inum);
> +	if (IS_ERR(inode)) {
> +		if (PTR_ERR(inode) == -ENOENT)
> +			return ERR_PTR(-ESTALE);
> +		return ERR_CAST(inode);
> +	}
> +
> +	dent = d_alloc_anon(inode);
> +	if (!dent) {
> +		iput(inode);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +	return dent;

I think you'd be better off using generic_fh_to_dentry/parent and
igoring the generation argument of the get_inode callback.  That way
the code is much smaller, and you're isolated from changes like the
d_alloc_anon to d_obtain_alias one.


> +/*
> + * Probably NFS support could be faster if other export operations were
> + * implemented, but '->fh_to_dentry()' is enough.
> + */
> +static struct export_operations ubifs_export_ops = {
> +	.fh_to_dentry = ubifs_fh_to_dentry,
> +};

No, it's not.  It seems to work with default options and when you don't
reboot the server.  You need a fh_to_parent and get_parent method, too.

> +	.fs_flags = FS_REQUIRES_DEV,

This one is wrong.

Do you have a patch in the tree somewhere to fix readdir for nfs?
Without that adding the export ops is a very bad idea.

Re: linux-next: ubifs tree build failure

[Christoph Hellwig]

On Tue, Aug 12, 2008 at 10:37:28AM +0300, Artem Bityutskiy wrote:
> ext Stephen Rothwell wrote:
> >Hi Artem,
> >
> >Today's linux-next build (x86_64 allmodconfig) failed like this:
> >
> >fs/ubifs/super.c: In function 'ubifs_fh_to_dentry':
> >fs/ubifs/super.c:1611: error: implicit declaration of function 
> >'d_alloc_anon'
> >fs/ubifs/super.c:1611: warning: assignment makes pointer from integer 
> >without a cast
> >
> >Caused by interaction between commit
> >95b53e84e8899258a1af011a7216abb978afae26 ("kill d_alloc_anon") from the
> >vfs tree and b51e4badb0248ac75fe3128bb369d987e28b2f88 ("UBIFS: add NFS
> >support") from the ubifs tree.  The former removes d_alloc_anon and the
> >latter adds a new usage.
> >
> >For now, I have reverted the ubifs commit.  Christoph, maybe you could
> >provide a better solution.
> >
> >I have appended the reverted patch below.
> 
> I am not sure how to resolve things like this actually. I am planning to
> send ubifs-2.6.git upstream soon, so probably Christoph will be able to 
> amend
> his stuff after this. Or any other suggestion?

Please back this patch out, it's defintively not ready.

Re: linux-next: ubifs tree build failure

[Christoph Hellwig]

Btw, a good example for export ops on a filesystem with similar
constraints is Dave's patch for jffs2:

	http://git.kernel.org/?p=linux/kernel/git/viro/vfs-2.6.git;a=commitdiff;h=dd9207c31d6712a4665f197db7740da6c633cea7

Re: linux-next: ubifs tree build failure

[Artem Bityutskiy]

ext Christoph Hellwig wrote:
> Btw, a good example for export ops on a filesystem with similar
> constraints is Dave's patch for jffs2:
> 
> 	http://git.kernel.org/?p=linux/kernel/git/viro/vfs-2.6.git;a=commitdiff;h=dd9207c31d6712a4665f197db7740da6c633cea7

OK, thanks.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: linux-next: ubifs tree build failure

[Artem Bityutskiy]

Christoph Hellwig wrote:
>>> I have appended the reverted patch below.
>> I am not sure how to resolve things like this actually. I am planning to
>> send ubifs-2.6.git upstream soon, so probably Christoph will be able to 
>> amend
>> his stuff after this. Or any other suggestion?
> 
> Please back this patch out, it's defintively not ready.

OK

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: linux-next: ubifs tree build failure

[Artem Bityutskiy]

Christoph Hellwig wrote:
> Btw, a good example for export ops on a filesystem with similar
> constraints is Dave's patch for jffs2:
> 
> 	http://git.kernel.org/?p=linux/kernel/git/viro/vfs-2.6.git;a=commitdiff;h=dd9207c31d6712a4665f197db7740da6c633cea7

OK, it is probably best if UBIFS NFS support goes via VFS git tree
just like JFFS2. I'll send a patch to fsdevel later.

Thus,  I'll remove NFS support from my tree and the conflict will
be solved.

Thanks.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: linux-next: ubifs tree build failure

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]

Hi Artem,

On Wed, 13 Aug 2008 12:58:06 +0300 Artem Bityutskiy <Artem.Bityutskiy@nokia.com> wrote:
>
> OK, it is probably best if UBIFS NFS support goes via VFS git tree
> just like JFFS2. I'll send a patch to fsdevel later.
> 
> Thus,  I'll remove NFS support from my tree and the conflict will
> be solved.

Thanks.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

linux-next: ubifs tree build failure

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 562 bytes --]

Hi Artem,

Today's linux-next build (x86_64 allmodconfig) failed like this:

fs/ubifs/super.c: In function 'open_ubi':
fs/ubifs/super.c:1866: error: implicit declaration of function 'ubi_open_volume_path'
fs/ubifs/super.c:1866: warning: assignment makes pointer from integer without a cast

Caused by commit 82ccbf8c4ec2113d4e73dbac3ca914019ca883ca ("UBIFS:
support mounting of UBI volume character devices") which I have reverted
for today.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: linux-next: ubifs tree build failure

[Artem Bityutskiy]

On Tue, 2009-11-24 at 02:14 +0100, Stephen Rothwell wrote:
> Hi Artem,
> 
> Today's linux-next build (x86_64 allmodconfig) failed like this:
> 
> fs/ubifs/super.c: In function 'open_ubi':
> fs/ubifs/super.c:1866: error: implicit declaration of function 'ubi_open_volume_path'
> fs/ubifs/super.c:1866: warning: assignment makes pointer from integer without a cast
> 
> Caused by commit 82ccbf8c4ec2113d4e73dbac3ca914019ca883ca ("UBIFS:
> support mounting of UBI volume character devices") which I have reverted
> for today.

Stephen,

thanks for quick reaction and sorry for the hassle. I've just fixed this
up.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: linux-next: ubifs tree build failure

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 323 bytes --]

Hi Artem,

On Tue, 24 Nov 2009 08:31:54 +0200 Artem Bityutskiy <Artem.Bityutskiy@nokia.com> wrote:
>
> thanks for quick reaction and sorry for the hassle. I've just fixed this
> up.

No worries and thanks.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]