[PATCH 3/6 linux-next] fs/affs: make affs exportable

[PATCH 3/6 linux-next] fs/affs: make affs exportable

[Fabian Frederick]

Add standard functions making AFFS work with NFS.

Functions based on ext4 implementation.
Tested on loop device.

Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
 fs/affs/affs.h  |  1 +
 fs/affs/namei.c | 40 ++++++++++++++++++++++++++++++++++++++++
 fs/affs/super.c |  1 +
 3 files changed, 42 insertions(+)

diff --git a/fs/affs/affs.h b/fs/affs/affs.h
index efe6839..1b55428 100644
--- a/fs/affs/affs.h
+++ b/fs/affs/affs.h
@@ -162,6 +162,7 @@ extern void	affs_free_bitmap(struct super_block *sb);
 
 /* namei.c */
 
+extern const struct export_operations affs_export_ops;
 extern int	affs_hash_name(struct super_block *sb, const u8 *name, unsigned int len);
 extern struct dentry *affs_lookup(struct inode *dir, struct dentry *dentry, unsigned int);
 extern int	affs_unlink(struct inode *dir, struct dentry *dentry);
diff --git a/fs/affs/namei.c b/fs/affs/namei.c
index 29186d2..04c3156f 100644
--- a/fs/affs/namei.c
+++ b/fs/affs/namei.c
@@ -9,6 +9,7 @@
  */
 
 #include "affs.h"
+#include <linux/exportfs.h>
 
 typedef int (*toupper_t)(int);
 
@@ -465,3 +466,42 @@ affs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	affs_brelse(bh);
 	return retval;
 }
+
+static struct inode *affs_nfs_get_inode(struct super_block *sb, u64 ino,
+					u32 generation)
+{
+	struct inode *inode;
+
+	if (!affs_validblock(sb, ino))
+		return ERR_PTR(-ESTALE);
+
+	inode = affs_iget(sb, ino);
+	if (IS_ERR(inode))
+		return ERR_CAST(inode);
+
+	if (generation && inode->i_generation != generation) {
+		iput(inode);
+		return ERR_PTR(-ESTALE);
+	}
+
+	return inode;
+}
+
+static struct dentry *affs_fh_to_dentry(struct super_block *sb, struct fid *fid,
+					int fh_len, int fh_type)
+{
+	return generic_fh_to_dentry(sb, fid, fh_len, fh_type,
+				    affs_nfs_get_inode);
+}
+
+static struct dentry *affs_fh_to_parent(struct super_block *sb, struct fid *fid,
+					int fh_len, int fh_type)
+{
+	return generic_fh_to_parent(sb, fid, fh_len, fh_type,
+				    affs_nfs_get_inode);
+}
+
+const struct export_operations affs_export_ops = {
+	.fh_to_dentry = affs_fh_to_dentry,
+	.fh_to_parent = affs_fh_to_parent,
+};
diff --git a/fs/affs/super.c b/fs/affs/super.c
index d638486..98bd952 100644
--- a/fs/affs/super.c
+++ b/fs/affs/super.c
@@ -507,6 +507,7 @@ static int affs_fill_super(struct super_block *sb, void *data, int silent)
 		return -ENOMEM;
 	}
 
+	sb->s_export_op = &affs_export_ops;
 	pr_debug("s_flags=%lX\n", sb->s_flags);
 	return 0;
 }
-- 
2.7.4

Re: [PATCH 3/6 linux-next] fs/affs: make affs exportable

[Al Viro]

On Tue, Jan 03, 2017 at 10:30:39PM +0100, Fabian Frederick wrote:
> Add standard functions making AFFS work with NFS.
> 
> Functions based on ext4 implementation.
> Tested on loop device.

How the hell is that supposed to work with cold dcache?  You don't have
->get_parent() there at all...

There *IS* a reference to parent directory in those suckers - not the same
kind as in normal unix filesystems (".." is not a directory entry there -
it's all fake), but it's doable.  be32_to_cpu(AFFS_TAIL(sb, bh)->parent) 
would be the inumber you need, where bh is the inode block of directory.

So it can be done, but not in this form.  NAK for the time being...

Re: [PATCH 3/6 linux-next] fs/affs: make affs exportable

[Fabian Frederick]

> On 03 January 2017 at 23:29 Al Viro <viro@ZenIV.linux.org.uk> wrote:
>
>
> On Tue, Jan 03, 2017 at 10:30:39PM +0100, Fabian Frederick wrote:
> > Add standard functions making AFFS work with NFS.
> >
> > Functions based on ext4 implementation.
> > Tested on loop device.
>
> How the hell is that supposed to work with cold dcache?  You don't have
> ->get_parent() there at all...
>
> There *IS* a reference to parent directory in those suckers - not the same
> kind as in normal unix filesystems (".." is not a directory entry there -
> it's all fake), but it's doable.  be32_to_cpu(AFFS_TAIL(sb, bh)->parent)
> would be the inumber you need, where bh is the inode block of directory.
>
> So it can be done, but not in this form.  NAK for the time being...

I worked on that function declared optional in
Documentation/filesystems/nfs/Exporting
but was unable to test it.

Regards,
Fabian

Re: [PATCH 3/6 linux-next] fs/affs: make affs exportable

[J. Bruce Fields]

On Wed, Jan 04, 2017 at 06:53:31AM +0100, Fabian Frederick wrote:
> 
> 
> > On 03 January 2017 at 23:29 Al Viro <viro@ZenIV.linux.org.uk> wrote:
> >
> >
> > On Tue, Jan 03, 2017 at 10:30:39PM +0100, Fabian Frederick wrote:
> > > Add standard functions making AFFS work with NFS.
> > >
> > > Functions based on ext4 implementation.
> > > Tested on loop device.
> >
> > How the hell is that supposed to work with cold dcache?  You don't have
> > ->get_parent() there at all...
> >
> > There *IS* a reference to parent directory in those suckers - not the same
> > kind as in normal unix filesystems (".." is not a directory entry there -
> > it's all fake), but it's doable.  be32_to_cpu(AFFS_TAIL(sb, bh)->parent)
> > would be the inumber you need, where bh is the inode block of directory.
> >
> > So it can be done, but not in this form.  NAK for the time being...
> 
> I worked on that function declared optional in
> Documentation/filesystems/nfs/Exporting
> but was unable to test it.

If we're going to reject patches that don't implement get_parent (and I
think we should), then we should replace "optional but strongly
recommended" there by just "mandatory".  Any objections?

--b.

Re: [PATCH 3/6 linux-next] fs/affs: make affs exportable

[Christoph Hellwig]

On Fri, Jan 13, 2017 at 12:39:12PM -0500, J. Bruce Fields wrote:
> If we're going to reject patches that don't implement get_parent (and I
> think we should), then we should replace "optional but strongly
> recommended" there by just "mandatory".  Any objections?

In theory yes - we'll just need an exception (or dummy implementation)
for in-memory file systems like tmpfs.

Re: [PATCH 3/6 linux-next] fs/affs: make affs exportable

[J. Bruce Fields]

On Fri, Jan 13, 2017 at 10:52:54AM -0800, Christoph Hellwig wrote:
> On Fri, Jan 13, 2017 at 12:39:12PM -0500, J. Bruce Fields wrote:
> > If we're going to reject patches that don't implement get_parent (and I
> > think we should), then we should replace "optional but strongly
> > recommended" there by just "mandatory".  Any objections?
> 
> In theory yes - we'll just need an exception (or dummy implementation)
> for in-memory file systems like tmpfs.

Hm, so does get_parent just never get called for tmpfs because the
parent's always there already?

Should be easy enough to document that exception.

--b.

Re: [PATCH 3/6 linux-next] fs/affs: make affs exportable

[Al Viro]

On Fri, Jan 13, 2017 at 02:03:57PM -0500, J. Bruce Fields wrote:
> On Fri, Jan 13, 2017 at 10:52:54AM -0800, Christoph Hellwig wrote:
> > On Fri, Jan 13, 2017 at 12:39:12PM -0500, J. Bruce Fields wrote:
> > > If we're going to reject patches that don't implement get_parent (and I
> > > think we should), then we should replace "optional but strongly
> > > recommended" there by just "mandatory".  Any objections?
> > 
> > In theory yes - we'll just need an exception (or dummy implementation)
> > for in-memory file systems like tmpfs.
> 
> Hm, so does get_parent just never get called for tmpfs because the
> parent's always there already?

	As the matter of fact, tmpfs *does* have a dummy ->get_parent().
FWIW, the situation right now looks so:
	1) most of the local filesystems: get_parent/fh_to_dentry/fh_to_parent.
efs, exofs, ext2, ext4, f2fs, fat, jffs2, jfs, ntfs, squashfs, udf, ufs are
that way.
	2) ditto, but with non-default ->encode_fh() (that's basically those
who need more than 32-bit inumber to get to an inode).  Often enough those
have non-default ->get_name() as well - default one relies upon in-core
->i_ino matching whatever getdents(2) puts into d_ino; besides, there might
be a faster way to search.  Ones without ->get_name(): fat_nostale, fuse,
isofs, nilfs2, ocfs2, reiserfs.  Ones with: lustre, btrfs, ceph, gfs2.
	3) shmem - essentially as (2), but there ->get_parent() is
a dummy and so's ->fh_to_parent(), the latter represented by NULL rather
than as an explicit stub.
	4) orangefs - no ->fh_to_parent, no ->get_parent.  Not sure if
orangefs has a way to get khandle of parent by that of directory (i.e.
if it can open a disconnected directory by khandle and then obtain
a khandle of parent from it).  They do include khandle of directory into
on-the-wire fhandle, but never actually use that part.  Might be blind
copying of what other fs are doing, might be something they plan to do
but hadn't gotten around to.  If it's the latter, that's yet another class
2 (and it'll definitely need a ->get_name() then).
	5) befs - ->fh_to_dentry/->fh_to_parent, no ->get_parent.  IIRC,
they simply don't have a way to find the parent by directory.
	6) cifs - stub ->get_parent(), NULL everything else, impossible
to use and what the hell is fs/cifs/export.c is doing in the tree, anyway?

Re: [PATCH 3/6 linux-next] fs/affs: make affs exportable

[J. Bruce Fields]

On Fri, Jan 13, 2017 at 07:57:06PM +0000, Al Viro wrote:
> On Fri, Jan 13, 2017 at 02:03:57PM -0500, J. Bruce Fields wrote:
> > On Fri, Jan 13, 2017 at 10:52:54AM -0800, Christoph Hellwig wrote:
> > > On Fri, Jan 13, 2017 at 12:39:12PM -0500, J. Bruce Fields wrote:
> > > > If we're going to reject patches that don't implement get_parent (and I
> > > > think we should), then we should replace "optional but strongly
> > > > recommended" there by just "mandatory".  Any objections?
> > > 
> > > In theory yes - we'll just need an exception (or dummy implementation)
> > > for in-memory file systems like tmpfs.
> > 
> > Hm, so does get_parent just never get called for tmpfs because the
> > parent's always there already?
> 
> 	As the matter of fact, tmpfs *does* have a dummy ->get_parent().
> FWIW, the situation right now looks so:

Thanks for the summary.

> 	1) most of the local filesystems: get_parent/fh_to_dentry/fh_to_parent.
> efs, exofs, ext2, ext4, f2fs, fat, jffs2, jfs, ntfs, squashfs, udf, ufs are
> that way.
> 	2) ditto, but with non-default ->encode_fh() (that's basically those
> who need more than 32-bit inumber to get to an inode).  Often enough those
> have non-default ->get_name() as well - default one relies upon in-core
> ->i_ino matching whatever getdents(2) puts into d_ino; besides, there might
> be a faster way to search.  Ones without ->get_name(): fat_nostale, fuse,
> isofs, nilfs2, ocfs2, reiserfs.  Ones with: lustre, btrfs, ceph, gfs2.
> 	3) shmem - essentially as (2), but there ->get_parent() is
> a dummy and so's ->fh_to_parent(), the latter represented by NULL rather
> than as an explicit stub.
> 	4) orangefs - no ->fh_to_parent, no ->get_parent.  Not sure if
> orangefs has a way to get khandle of parent by that of directory (i.e.
> if it can open a disconnected directory by khandle and then obtain
> a khandle of parent from it).  They do include khandle of directory into
> on-the-wire fhandle, but never actually use that part.  Might be blind
> copying of what other fs are doing, might be something they plan to do
> but hadn't gotten around to.  If it's the latter, that's yet another class
> 2 (and it'll definitely need a ->get_name() then).
> 	5) befs - ->fh_to_dentry/->fh_to_parent, no ->get_parent.  IIRC,
> they simply don't have a way to find the parent by directory.
> 	6) cifs - stub ->get_parent(), NULL everything else, impossible
> to use and what the hell is fs/cifs/export.c is doing in the tree, anyway?

Dunno, I thought I tried to remove it once some time ago, but my memory
may be wrong.  It seems like nothing more than a trap for the unwary.

--b.

Re: [PATCH 3/6 linux-next] fs/affs: make affs exportable

[Fabian Frederick]

> On 03 January 2017 at 23:29 Al Viro <viro@ZenIV.linux.org.uk> wrote:
>
>
> On Tue, Jan 03, 2017 at 10:30:39PM +0100, Fabian Frederick wrote:
> > Add standard functions making AFFS work with NFS.
> >
> > Functions based on ext4 implementation.
> > Tested on loop device.
>
> How the hell is that supposed to work with cold dcache?  You don't have
> ->get_parent() there at all...
>
> There *IS* a reference to parent directory in those suckers - not the same
> kind as in normal unix filesystems (".." is not a directory entry there -
> it's all fake), but it's doable.  be32_to_cpu(AFFS_TAIL(sb, bh)->parent)
> would be the inumber you need, where bh is the inode block of directory.
>
> So it can be done, but not in this form.  NAK for the time being...
I tried with the following function:

static struct dentry *affs_get_parent(struct dentry *child)
{
        struct inode *parent;
        struct buffer_head *bh;

        bh = affs_bread(child->d_sb, d_inode(child)->i_ino);
        if (IS_ERR(bh))
                return ERR_CAST(bh);

        parent = affs_iget(child->d_sb,
                           be32_to_cpu(AFFS_TAIL(child->d_sb, bh)->parent));
        brelse(bh);
        if (IS_ERR(parent))
                return ERR_CAST(parent);

        return d_obtain_alias(parent);
}

but after creating a new file and echo 3 > /proc/sys/vm/drop_caches
on server, client directory requires some seconds before being updated
or gives "ls: cannot open directory '.': Stale file handle" the first time
and correct directory when trying again... Do you know how I could improve this
?

Regards,
Fabian