[PATCH 0/6] UBIFS NFS export support

[PATCH 0/6] UBIFS NFS export support

[Richard Weinberger]

Adding fscrypto support to UBIFS forced us to implement 64bit
cookies. Using this building block we can finally implement
export operations and allow UBIFS being exported via NFS.
This series adds the last missing bits.

It can be obtained from:
git://git.infradead.org/users/rw/linux.git ubifs_export_v1

Richard Weinberger (6):
  ext4: Move is_32bit_api() to generic code
  ubifs: Provide a custom llseek for directories
  ubifs: Use 64bit readdir cookies
  ubifs: Maintain a parent pointer
  ubifs: Implement export_operations
  ubifs: Wire up NFS support

 fs/ext4/dir.c          |  9 ------
 fs/ubifs/dir.c         | 88 ++++++++++++++++++++++++++++++++++++++------------
 fs/ubifs/journal.c     |  5 ++-
 fs/ubifs/key.h         | 59 +++++++++++++++++++++++++++++++++
 fs/ubifs/sb.c          |  2 ++
 fs/ubifs/super.c       | 35 ++++++++++++++++++++
 fs/ubifs/ubifs-media.h | 18 +++++++++--
 fs/ubifs/ubifs.h       |  6 ++++
 include/linux/compat.h |  2 ++
 9 files changed, 190 insertions(+), 34 deletions(-)

-- 
2.7.3

[PATCH 1/6] ext4: Move is_32bit_api() to generic code

[Richard Weinberger]

Since UBIFS will also use this function, move it to compat.h.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/ext4/dir.c          | 9 ---------
 include/linux/compat.h | 2 ++
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
index e8b365000d73..464e6e99d744 100644
--- a/fs/ext4/dir.c
+++ b/fs/ext4/dir.c
@@ -290,15 +290,6 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx)
 	return err;
 }
 
-static inline int is_32bit_api(void)
-{
-#ifdef CONFIG_COMPAT
-	return in_compat_syscall();
-#else
-	return (BITS_PER_LONG == 32);
-#endif
-}
-
 /*
  * These functions convert from the major/minor hash to an f_pos
  * value for dx directories
diff --git a/include/linux/compat.h b/include/linux/compat.h
index 63609398ef9f..8412382e8d08 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -730,11 +730,13 @@ asmlinkage long compat_sys_fanotify_mark(int, unsigned int, __u32, __u32,
 #ifndef in_compat_syscall
 static inline bool in_compat_syscall(void) { return is_compat_task(); }
 #endif
+static inline int is_32bit_api(void) { return in_compat_syscall(); }
 
 #else
 
 #define is_compat_task() (0)
 static inline bool in_compat_syscall(void) { return false; }
+static inline int is_32bit_api(void) { return BITS_PER_LONG == 32; }
 
 #endif /* CONFIG_COMPAT */
 
-- 
2.7.3

Re: [PATCH 1/6] ext4: Move is_32bit_api() to generic code

[Richard Weinberger]

On Thu, Dec 1, 2016 at 11:02 PM, Richard Weinberger <richard@nod.at> wrote:
> Since UBIFS will also use this function, move it to compat.h.
>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  fs/ext4/dir.c          | 9 ---------
>  include/linux/compat.h | 2 ++
>  2 files changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
> index e8b365000d73..464e6e99d744 100644
> --- a/fs/ext4/dir.c
> +++ b/fs/ext4/dir.c
> @@ -290,15 +290,6 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx)
>         return err;
>  }
>
> -static inline int is_32bit_api(void)
> -{
> -#ifdef CONFIG_COMPAT
> -       return in_compat_syscall();
> -#else
> -       return (BITS_PER_LONG == 32);
> -#endif
> -}
> -
>  /*
>   * These functions convert from the major/minor hash to an f_pos
>   * value for dx directories
> diff --git a/include/linux/compat.h b/include/linux/compat.h
> index 63609398ef9f..8412382e8d08 100644
> --- a/include/linux/compat.h
> +++ b/include/linux/compat.h
> @@ -730,11 +730,13 @@ asmlinkage long compat_sys_fanotify_mark(int, unsigned int, __u32, __u32,
>  #ifndef in_compat_syscall
>  static inline bool in_compat_syscall(void) { return is_compat_task(); }
>  #endif
> +static inline int is_32bit_api(void) { return in_compat_syscall(); }
>
>  #else
>
>  #define is_compat_task() (0)
>  static inline bool in_compat_syscall(void) { return false; }
> +static inline int is_32bit_api(void) { return BITS_PER_LONG == 32; }
>
>  #endif /* CONFIG_COMPAT */

Just realized that this patch didn't get any feedback.
Ted, are you fine with it?

Anyway, exportable UBIFS has to wait until the next merge window.

-- 
Thanks,
//richard

Re: [PATCH 1/6] ext4: Move is_32bit_api() to generic code

[Andreas Dilger]

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

On Dec 1, 2016, at 3:02 PM, Richard Weinberger <richard@nod.at> wrote:
> 
> Since UBIFS will also use this function, move it to compat.h.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>

Looks fine, could have minor improvement as mentioned below.

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> ---
> fs/ext4/dir.c          | 9 ---------
> include/linux/compat.h | 2 ++
> 2 files changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
> index e8b365000d73..464e6e99d744 100644
> --- a/fs/ext4/dir.c
> +++ b/fs/ext4/dir.c
> @@ -290,15 +290,6 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx)
> 	return err;
> }
> 
> -static inline int is_32bit_api(void)
> -{
> -#ifdef CONFIG_COMPAT
> -	return in_compat_syscall();
> -#else
> -	return (BITS_PER_LONG == 32);
> -#endif
> -}
> -
> /*
>  * These functions convert from the major/minor hash to an f_pos
>  * value for dx directories
> diff --git a/include/linux/compat.h b/include/linux/compat.h
> index 63609398ef9f..8412382e8d08 100644
> --- a/include/linux/compat.h
> +++ b/include/linux/compat.h
> @@ -730,11 +730,13 @@ asmlinkage long compat_sys_fanotify_mark(int, unsigned int, __u32, __u32,
> #ifndef in_compat_syscall
> static inline bool in_compat_syscall(void) { return is_compat_task(); }
> #endif
> +static inline int is_32bit_api(void) { return in_compat_syscall(); }
> 
> #else

This would benefit from an annotation to make it clear what the "#if" is:

#else /* !CONFIG_COMPAT */

> 
> #define is_compat_task() (0)
> static inline bool in_compat_syscall(void) { return false; }
> +static inline int is_32bit_api(void) { return BITS_PER_LONG == 32; }
> 
> #endif /* CONFIG_COMPAT */


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/6] ubifs: Provide a custom llseek for directories

[Richard Weinberger]

When UBIFS is using 64bit cookies for directory offsets,
we have to support seeking that deep into direcoties.
Since generic_file_llseek() is uses sb->s_maxbytes it will
fail for use because on ubifs sb->s_maxbytes is the real maxmal
file size which can be much smaller than 64bit due to limitations
of the flash device.

Therefore provie a custom llseek operation.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/ubifs/dir.c         | 12 +++++++++++-
 fs/ubifs/ubifs-media.h |  6 ++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 7f01f3d2ac3b..883b2fdf51df 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -1721,6 +1721,16 @@ static int ubifs_dir_open(struct inode *dir, struct file *file)
 	return 0;
 }
 
+static loff_t ubifs_dir_llseek(struct file *file, loff_t offset, int whence)
+{
+	struct inode *inode = file->f_mapping->host;
+
+	return generic_file_llseek_size(file, offset, whence,
+					(loff_t)UBIFS_S_KEY_HASH_MASK <<
+					UBIFS_DH_BITS | UBIFS_DH_MASK,
+					i_size_read(inode));
+}
+
 const struct inode_operations ubifs_dir_inode_operations = {
 	.lookup      = ubifs_lookup,
 	.create      = ubifs_create,
@@ -1741,7 +1751,7 @@ const struct inode_operations ubifs_dir_inode_operations = {
 };
 
 const struct file_operations ubifs_dir_operations = {
-	.llseek         = generic_file_llseek,
+	.llseek         = ubifs_dir_llseek,
 	.release        = ubifs_dir_release,
 	.read           = generic_read_dir,
 	.iterate_shared = ubifs_readdir,
diff --git a/fs/ubifs/ubifs-media.h b/fs/ubifs/ubifs-media.h
index e8c23c9d4f4a..5939776c7359 100644
--- a/fs/ubifs/ubifs-media.h
+++ b/fs/ubifs/ubifs-media.h
@@ -214,6 +214,12 @@ enum {
 #define UBIFS_S_KEY_HASH_MASK  UBIFS_S_KEY_BLOCK_MASK
 
 /*
+ * We use 32 bits for the double hash cookie.
+ */
+#define UBIFS_DH_MASK 0xffffffff
+#define UBIFS_DH_BITS 32
+
+/*
  * Key types.
  *
  * UBIFS_INO_KEY: inode node key
-- 
2.7.3

[PATCH 3/6] ubifs: Use 64bit readdir cookies

[Richard Weinberger]

This is the first step to support proper telldir/seekdir()
in UBIFS.
Let's report 64bit cookies in readdir(). The cookie is a combination
of the entry key plus the double hash value.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/ubifs/dir.c   | 46 +++++++++++++++++++++++++++++++------------
 fs/ubifs/key.h   | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/ubifs/ubifs.h |  1 +
 3 files changed, 94 insertions(+), 12 deletions(-)

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 883b2fdf51df..3b8c08dad75b 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -539,7 +539,7 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx)
 
 	dbg_gen("dir ino %lu, f_pos %#llx", dir->i_ino, ctx->pos);
 
-	if (ctx->pos > UBIFS_S_KEY_HASH_MASK || ctx->pos == 2)
+	if (ctx->pos == 2)
 		/*
 		 * The directory was seek'ed to a senseless position or there
 		 * are no more entries.
@@ -594,7 +594,7 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx)
 			goto out;
 		}
 
-		ctx->pos = key_hash_flash(c, &dent->key);
+		ctx->pos = key_get_dir_pos(c, file, dent);
 		file->private_data = dent;
 	}
 
@@ -604,21 +604,43 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx)
 		 * The directory was seek'ed to and is now readdir'ed.
 		 * Find the entry corresponding to @ctx->pos or the closest one.
 		 */
-		dent_key_init_hash(c, &key, dir->i_ino, ctx->pos);
-		fname_len(&nm) = 0;
-		dent = ubifs_tnc_next_ent(c, &key, &nm);
-		if (IS_ERR(dent)) {
-			err = PTR_ERR(dent);
-			goto out;
+		dent_key_init_hash(c, &key, dir->i_ino,
+				   key_get_hash_from_dir_pos(c, file, ctx->pos));
+
+		if (key_want_short_hash(file)) {
+			err = -ENOENT;
+		} else {
+			dent = kmalloc(UBIFS_MAX_DENT_NODE_SZ, GFP_NOFS);
+			if (!dent) {
+				err = -ENOMEM;
+				goto out;
+			}
+
+			err = ubifs_tnc_lookup_dh(c, &key, dent,
+				key_get_cookie_from_dir_pos(c, ctx->pos));
+		}
+		if (err) {
+			kfree(dent);
+
+			if (err < 0 && err != -ENOENT && err != -EOPNOTSUPP)
+				goto out;
+
+			fname_len(&nm) = 0;
+			dent = ubifs_tnc_next_ent(c, &key, &nm);
+			if (IS_ERR(dent)) {
+				err = PTR_ERR(dent);
+				goto out;
+			}
 		}
-		ctx->pos = key_hash_flash(c, &dent->key);
+
+		ctx->pos = key_get_dir_pos(c, file, dent);
 		file->private_data = dent;
 	}
 
 	while (1) {
-		dbg_gen("feed '%s', ino %llu, new f_pos %#x",
+		dbg_gen("feed '%s', ino %llu, new f_pos %#lx",
 			dent->name, (unsigned long long)le64_to_cpu(dent->inum),
-			key_hash_flash(c, &dent->key));
+			(unsigned long)key_get_dir_pos(c, file, dent));
 		ubifs_assert(le64_to_cpu(dent->ch.sqnum) >
 			     ubifs_inode(dir)->creat_sqnum);
 
@@ -656,7 +678,7 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx)
 		}
 
 		kfree(file->private_data);
-		ctx->pos = key_hash_flash(c, &dent->key);
+		ctx->pos = key_get_dir_pos(c, file, dent);
 		file->private_data = dent;
 		cond_resched();
 	}
diff --git a/fs/ubifs/key.h b/fs/ubifs/key.h
index 7547be512db2..2788e36ce832 100644
--- a/fs/ubifs/key.h
+++ b/fs/ubifs/key.h
@@ -397,6 +397,65 @@ static inline uint32_t key_hash_flash(const struct ubifs_info *c, const void *k)
 }
 
 /**
+ * key_want_short_hash - tests whether we can emit a 64bit hash or not.
+ * @file: the file handle of the directory
+ */
+static inline bool key_want_short_hash(struct file *file)
+{
+	if (file->f_mode & FMODE_32BITHASH)
+		return true;
+
+	if (!(file->f_mode & FMODE_64BITHASH) && is_32bit_api())
+		return true;
+
+	return false;
+}
+
+/**
+ * key_dir_pos - compute a 64bit directory cookie for readdir()
+ * @c: UBIFS file-system description object
+ * @file: the file handle of the directory
+ * @dent: the directory entry
+ */
+static inline loff_t key_get_dir_pos(const struct ubifs_info *c,
+				     struct file *file,
+				     struct ubifs_dent_node *dent)
+{
+	BUILD_BUG_ON(sizeof(loff_t) < 8);
+
+	if (key_want_short_hash(file))
+		return key_hash_flash(c, &dent->key);
+
+	return ((loff_t)key_hash_flash(c, &dent->key)) << UBIFS_DH_BITS | le32_to_cpu(dent->cookie);
+}
+
+/**
+ * key_get_hash_from_dir_pos - extracts the flash key from a directory offset.
+ * @c: UBIFS file-system description object
+ * @file: the file handle of the directory
+ * @pos: the directory offset provied by VFS
+ */
+static inline uint32_t key_get_hash_from_dir_pos(const struct ubifs_info *c,
+						 struct file *file, loff_t pos)
+{
+	if (key_want_short_hash(file))
+		return pos & UBIFS_S_KEY_HASH_MASK;
+
+	return (pos >> UBIFS_DH_BITS) & UBIFS_S_KEY_HASH_MASK;
+}
+
+/**
+ * key_get_cookie_from_dir_pos - extracts the double hash cookie from a directory offset.
+ * @c: UBIFS file-system description object
+ * @pos: the directory offset provied by VFS
+ */
+static inline uint32_t key_get_cookie_from_dir_pos(const struct ubifs_info *c,
+						   loff_t pos)
+{
+	return pos & UBIFS_DH_MASK;
+}
+
+/**
  * key_block - get data block number.
  * @c: UBIFS file-system description object
  * @key: the key to get the block number from
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index 12f3df3ced0e..0532a6f82b1d 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -40,6 +40,7 @@
 #include <linux/xattr.h>
 #include <linux/fscrypto.h>
 #include <linux/random.h>
+#include <linux/compat.h>
 #include "ubifs-media.h"
 
 /* Version of this UBIFS implementation */
-- 
2.7.3

Re: [PATCH 3/6] ubifs: Use 64bit readdir cookies

[J. Bruce Fields]

On Thu, Dec 01, 2016 at 11:02:18PM +0100, Richard Weinberger wrote:
> This is the first step to support proper telldir/seekdir()
> in UBIFS.
> Let's report 64bit cookies in readdir(). The cookie is a combination
> of the entry key plus the double hash value.

Would it be possible to explain what that means in a little detail, for
a ubifs-ignoramus?

I'm just curious how it meets the requirements for nfs exports.

--b.

> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  fs/ubifs/dir.c   | 46 +++++++++++++++++++++++++++++++------------
>  fs/ubifs/key.h   | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/ubifs/ubifs.h |  1 +
>  3 files changed, 94 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> index 883b2fdf51df..3b8c08dad75b 100644
> --- a/fs/ubifs/dir.c
> +++ b/fs/ubifs/dir.c
> @@ -539,7 +539,7 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx)
>  
>  	dbg_gen("dir ino %lu, f_pos %#llx", dir->i_ino, ctx->pos);
>  
> -	if (ctx->pos > UBIFS_S_KEY_HASH_MASK || ctx->pos == 2)
> +	if (ctx->pos == 2)
>  		/*
>  		 * The directory was seek'ed to a senseless position or there
>  		 * are no more entries.
> @@ -594,7 +594,7 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx)
>  			goto out;
>  		}
>  
> -		ctx->pos = key_hash_flash(c, &dent->key);
> +		ctx->pos = key_get_dir_pos(c, file, dent);
>  		file->private_data = dent;
>  	}
>  
> @@ -604,21 +604,43 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx)
>  		 * The directory was seek'ed to and is now readdir'ed.
>  		 * Find the entry corresponding to @ctx->pos or the closest one.
>  		 */
> -		dent_key_init_hash(c, &key, dir->i_ino, ctx->pos);
> -		fname_len(&nm) = 0;
> -		dent = ubifs_tnc_next_ent(c, &key, &nm);
> -		if (IS_ERR(dent)) {
> -			err = PTR_ERR(dent);
> -			goto out;
> +		dent_key_init_hash(c, &key, dir->i_ino,
> +				   key_get_hash_from_dir_pos(c, file, ctx->pos));
> +
> +		if (key_want_short_hash(file)) {
> +			err = -ENOENT;
> +		} else {
> +			dent = kmalloc(UBIFS_MAX_DENT_NODE_SZ, GFP_NOFS);
> +			if (!dent) {
> +				err = -ENOMEM;
> +				goto out;
> +			}
> +
> +			err = ubifs_tnc_lookup_dh(c, &key, dent,
> +				key_get_cookie_from_dir_pos(c, ctx->pos));
> +		}
> +		if (err) {
> +			kfree(dent);
> +
> +			if (err < 0 && err != -ENOENT && err != -EOPNOTSUPP)
> +				goto out;
> +
> +			fname_len(&nm) = 0;
> +			dent = ubifs_tnc_next_ent(c, &key, &nm);
> +			if (IS_ERR(dent)) {
> +				err = PTR_ERR(dent);
> +				goto out;
> +			}
>  		}
> -		ctx->pos = key_hash_flash(c, &dent->key);
> +
> +		ctx->pos = key_get_dir_pos(c, file, dent);
>  		file->private_data = dent;
>  	}
>  
>  	while (1) {
> -		dbg_gen("feed '%s', ino %llu, new f_pos %#x",
> +		dbg_gen("feed '%s', ino %llu, new f_pos %#lx",
>  			dent->name, (unsigned long long)le64_to_cpu(dent->inum),
> -			key_hash_flash(c, &dent->key));
> +			(unsigned long)key_get_dir_pos(c, file, dent));
>  		ubifs_assert(le64_to_cpu(dent->ch.sqnum) >
>  			     ubifs_inode(dir)->creat_sqnum);
>  
> @@ -656,7 +678,7 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx)
>  		}
>  
>  		kfree(file->private_data);
> -		ctx->pos = key_hash_flash(c, &dent->key);
> +		ctx->pos = key_get_dir_pos(c, file, dent);
>  		file->private_data = dent;
>  		cond_resched();
>  	}
> diff --git a/fs/ubifs/key.h b/fs/ubifs/key.h
> index 7547be512db2..2788e36ce832 100644
> --- a/fs/ubifs/key.h
> +++ b/fs/ubifs/key.h
> @@ -397,6 +397,65 @@ static inline uint32_t key_hash_flash(const struct ubifs_info *c, const void *k)
>  }
>  
>  /**
> + * key_want_short_hash - tests whether we can emit a 64bit hash or not.
> + * @file: the file handle of the directory
> + */
> +static inline bool key_want_short_hash(struct file *file)
> +{
> +	if (file->f_mode & FMODE_32BITHASH)
> +		return true;
> +
> +	if (!(file->f_mode & FMODE_64BITHASH) && is_32bit_api())
> +		return true;
> +
> +	return false;
> +}
> +
> +/**
> + * key_dir_pos - compute a 64bit directory cookie for readdir()
> + * @c: UBIFS file-system description object
> + * @file: the file handle of the directory
> + * @dent: the directory entry
> + */
> +static inline loff_t key_get_dir_pos(const struct ubifs_info *c,
> +				     struct file *file,
> +				     struct ubifs_dent_node *dent)
> +{
> +	BUILD_BUG_ON(sizeof(loff_t) < 8);
> +
> +	if (key_want_short_hash(file))
> +		return key_hash_flash(c, &dent->key);
> +
> +	return ((loff_t)key_hash_flash(c, &dent->key)) << UBIFS_DH_BITS | le32_to_cpu(dent->cookie);
> +}
> +
> +/**
> + * key_get_hash_from_dir_pos - extracts the flash key from a directory offset.
> + * @c: UBIFS file-system description object
> + * @file: the file handle of the directory
> + * @pos: the directory offset provied by VFS
> + */
> +static inline uint32_t key_get_hash_from_dir_pos(const struct ubifs_info *c,
> +						 struct file *file, loff_t pos)
> +{
> +	if (key_want_short_hash(file))
> +		return pos & UBIFS_S_KEY_HASH_MASK;
> +
> +	return (pos >> UBIFS_DH_BITS) & UBIFS_S_KEY_HASH_MASK;
> +}
> +
> +/**
> + * key_get_cookie_from_dir_pos - extracts the double hash cookie from a directory offset.
> + * @c: UBIFS file-system description object
> + * @pos: the directory offset provied by VFS
> + */
> +static inline uint32_t key_get_cookie_from_dir_pos(const struct ubifs_info *c,
> +						   loff_t pos)
> +{
> +	return pos & UBIFS_DH_MASK;
> +}
> +
> +/**
>   * key_block - get data block number.
>   * @c: UBIFS file-system description object
>   * @key: the key to get the block number from
> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
> index 12f3df3ced0e..0532a6f82b1d 100644
> --- a/fs/ubifs/ubifs.h
> +++ b/fs/ubifs/ubifs.h
> @@ -40,6 +40,7 @@
>  #include <linux/xattr.h>
>  #include <linux/fscrypto.h>
>  #include <linux/random.h>
> +#include <linux/compat.h>
>  #include "ubifs-media.h"
>  
>  /* Version of this UBIFS implementation */
> -- 
> 2.7.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] ubifs: Use 64bit readdir cookies

[Richard Weinberger]

Bruce,

On 29.12.2016 03:58, J. Bruce Fields wrote:
> On Thu, Dec 01, 2016 at 11:02:18PM +0100, Richard Weinberger wrote:
>> This is the first step to support proper telldir/seekdir()
>> in UBIFS.
>> Let's report 64bit cookies in readdir(). The cookie is a combination
>> of the entry key plus the double hash value.
> 
> Would it be possible to explain what that means in a little detail, for
> a ubifs-ignoramus?
> 
> I'm just curious how it meets the requirements for nfs exports.

Traditionally UBIFS had 32bit readdir() cookies, ctx->pos was a 32bit
integer.
This integer is the 32bit key out the UBIFS index tree.

In ->lookup(), UBIFS computes the r5 hash of the requested file name
which will be used as search key. Since the chance is high to face a hash
collision in the 32bit space, UBIFS also does a string compare
to find the correct directory entry for the given file name.

For NFS, and fscrypt, UBIFS has to offer a way to lookup a directory
entry by a given cookie without knowing the file name.
So, UBIFS has no chance to detect or handle a hash collision.

To deal with that UBIFS uses a similar trick as ext4 does, it stores
another unique identifier of the file name which can be used as cookie.
While ext4 stores two 32bit hash values, therefore the name double hash,
which will be combined to a single 64bit cookie, UBIFS stores additionally
a 32bit random number which will be generated upon directory entry creation.
Using the 32bit hash value and the 32bit nonce it can provide a 64bit
cookie.

Lookup via cookie works like a regular lookup but instead of comparing
strings it compares the nonce values.

That way UBIFS can provide a 64bit readdir() cookie which is required for NFS3.

Thanks,
//richard

Re: [PATCH 3/6] ubifs: Use 64bit readdir cookies

[J. Bruce Fields]

On Thu, Dec 29, 2016 at 10:19:27AM +0100, Richard Weinberger wrote:
> Bruce,
> 
> On 29.12.2016 03:58, J. Bruce Fields wrote:
> > On Thu, Dec 01, 2016 at 11:02:18PM +0100, Richard Weinberger wrote:
> >> This is the first step to support proper telldir/seekdir()
> >> in UBIFS.
> >> Let's report 64bit cookies in readdir(). The cookie is a combination
> >> of the entry key plus the double hash value.
> > 
> > Would it be possible to explain what that means in a little detail, for
> > a ubifs-ignoramus?
> > 
> > I'm just curious how it meets the requirements for nfs exports.
> 
> Traditionally UBIFS had 32bit readdir() cookies, ctx->pos was a 32bit
> integer.
> This integer is the 32bit key out the UBIFS index tree.
> 
> In ->lookup(), UBIFS computes the r5 hash of the requested file name
> which will be used as search key. Since the chance is high to face a hash
> collision in the 32bit space, UBIFS also does a string compare
> to find the correct directory entry for the given file name.
> 
> For NFS, and fscrypt, UBIFS has to offer a way to lookup a directory
> entry by a given cookie without knowing the file name.
> So, UBIFS has no chance to detect or handle a hash collision.
> 
> To deal with that UBIFS uses a similar trick as ext4 does, it stores
> another unique identifier of the file name which can be used as cookie.
> While ext4 stores two 32bit hash values, therefore the name double hash,
> which will be combined to a single 64bit cookie, UBIFS stores additionally
> a 32bit random number which will be generated upon directory entry creation.
> Using the 32bit hash value and the 32bit nonce it can provide a 64bit
> cookie.
> 
> Lookup via cookie works like a regular lookup but instead of comparing
> strings it compares the nonce values.
> 
> That way UBIFS can provide a 64bit readdir() cookie which is required for NFS3.

Sounds good.  And if a matching entry isn't found (as in the case of a
concurrent unlink), what happens?  The answer must be the same as for
ext4, but I've forgotten the details....  I guess it must find the next
highest cookie (thinking of the cookie as a 64-bit integer of some kind)
that exists in the directory.  And that must be the same order that
readdir normally returns entries in.

--b.

Re: [PATCH 3/6] ubifs: Use 64bit readdir cookies

[Richard Weinberger]

Bruce,

On 29.12.2016 16:34, J. Bruce Fields wrote:
>> That way UBIFS can provide a 64bit readdir() cookie which is required for NFS3.
> 
> Sounds good.  And if a matching entry isn't found (as in the case of a
> concurrent unlink), what happens?  The answer must be the same as for
> ext4, but I've forgotten the details....  I guess it must find the next
> highest cookie (thinking of the cookie as a 64-bit integer of some kind)
> that exists in the directory.  And that must be the same order that
> readdir normally returns entries in.

If a 64bit cookie is not found, the lookup function returns -ENOENT.
In UBIFS we cannot just select a higher or lower key (cookie in this case),
since it is the B-tree key and would point to a completely different
entry.

So, in case of a concurrent unlink() one would succeed and one fail with
-ENOENT. Unless I miss something that seems okay to me.

Thanks,
//richard

Re: [PATCH 3/6] ubifs: Use 64bit readdir cookies

[J. Bruce Fields]

On Thu, Dec 29, 2016 at 04:49:54PM +0100, Richard Weinberger wrote:
> Bruce,
> 
> On 29.12.2016 16:34, J. Bruce Fields wrote:
> >> That way UBIFS can provide a 64bit readdir() cookie which is required for NFS3.
> > 
> > Sounds good.  And if a matching entry isn't found (as in the case of a
> > concurrent unlink), what happens?  The answer must be the same as for
> > ext4, but I've forgotten the details....  I guess it must find the next
> > highest cookie (thinking of the cookie as a 64-bit integer of some kind)
> > that exists in the directory.  And that must be the same order that
> > readdir normally returns entries in.
> 
> If a 64bit cookie is not found, the lookup function returns -ENOENT.
> In UBIFS we cannot just select a higher or lower key (cookie in this case),
> since it is the B-tree key and would point to a completely different
> entry.
> 
> So, in case of a concurrent unlink() one would succeed and one fail with
> -ENOENT. Unless I miss something that seems okay to me.

Unlink takes (parent directory, name), not a directory cookie.

The problem is concurrent unlink and nfs readdir.  So:

	NFS server returns readdir result with cookie X

	Somebody unlinks the entry at X.

	NFS server gets readdir request with cookie X.

Then the NFS client will get a spurious -ENOENT.

I'm not sure how best to reproduce that.... Maybe:

	Create a directory on an nfs-exported filesystem with lots of
	entries.

	Start a loop (or loops?) renaming directory entries within the
	directory as fast as possible (or deleting and creating entries;
	I assume it's the same thing for our purposes).

	read the directory from an nfs client.

I'm not sure how many entries is "lots".... Ideally you want a single
read of the directory to require the client to make lots of READDIR
requests to the server.  You could help by running:

	echo 1024 >/proc/fs/nfsd/max_block_size

before starting knfsd.  That should force it to return no more than 1K
of data in each READDIR reply.

--b.

Re: [PATCH 3/6] ubifs: Use 64bit readdir cookies

[Richard Weinberger]

Bruce,

On 29.12.2016 17:15, J. Bruce Fields wrote:
> On Thu, Dec 29, 2016 at 04:49:54PM +0100, Richard Weinberger wrote:
>> Bruce,
>>
>> On 29.12.2016 16:34, J. Bruce Fields wrote:
>>>> That way UBIFS can provide a 64bit readdir() cookie which is required for NFS3.
>>>
>>> Sounds good.  And if a matching entry isn't found (as in the case of a
>>> concurrent unlink), what happens?  The answer must be the same as for
>>> ext4, but I've forgotten the details....  I guess it must find the next
>>> highest cookie (thinking of the cookie as a 64-bit integer of some kind)
>>> that exists in the directory.  And that must be the same order that
>>> readdir normally returns entries in.
>>
>> If a 64bit cookie is not found, the lookup function returns -ENOENT.
>> In UBIFS we cannot just select a higher or lower key (cookie in this case),
>> since it is the B-tree key and would point to a completely different
>> entry.
>>
>> So, in case of a concurrent unlink() one would succeed and one fail with
>> -ENOENT. Unless I miss something that seems okay to me.
> 
> Unlink takes (parent directory, name), not a directory cookie.
> 
> The problem is concurrent unlink and nfs readdir.  So:
> 
> 	NFS server returns readdir result with cookie X
> 
> 	Somebody unlinks the entry at X.
> 
> 	NFS server gets readdir request with cookie X.
> 
> Then the NFS client will get a spurious -ENOENT.

Ah yes. Sorry I misunderstood your question.
UBIFS readdir() address this already, if you ask it to readdir()
from pos X and X is not present it will jump to the next best entry X'.
UBIFS does so since ever.

Thanks,
//richard

Re: [PATCH 3/6] ubifs: Use 64bit readdir cookies

[J. Bruce Fields]

On Thu, Dec 29, 2016 at 05:36:35PM +0100, Richard Weinberger wrote:
> Bruce,
> 
> On 29.12.2016 17:15, J. Bruce Fields wrote:
> > On Thu, Dec 29, 2016 at 04:49:54PM +0100, Richard Weinberger wrote:
> >> Bruce,
> >>
> >> On 29.12.2016 16:34, J. Bruce Fields wrote:
> >>>> That way UBIFS can provide a 64bit readdir() cookie which is required for NFS3.
> >>>
> >>> Sounds good.  And if a matching entry isn't found (as in the case of a
> >>> concurrent unlink), what happens?  The answer must be the same as for
> >>> ext4, but I've forgotten the details....  I guess it must find the next
> >>> highest cookie (thinking of the cookie as a 64-bit integer of some kind)
> >>> that exists in the directory.  And that must be the same order that
> >>> readdir normally returns entries in.
> >>
> >> If a 64bit cookie is not found, the lookup function returns -ENOENT.
> >> In UBIFS we cannot just select a higher or lower key (cookie in this case),
> >> since it is the B-tree key and would point to a completely different
> >> entry.
> >>
> >> So, in case of a concurrent unlink() one would succeed and one fail with
> >> -ENOENT. Unless I miss something that seems okay to me.
> > 
> > Unlink takes (parent directory, name), not a directory cookie.
> > 
> > The problem is concurrent unlink and nfs readdir.  So:
> > 
> > 	NFS server returns readdir result with cookie X
> > 
> > 	Somebody unlinks the entry at X.
> > 
> > 	NFS server gets readdir request with cookie X.
> > 
> > Then the NFS client will get a spurious -ENOENT.
> 
> Ah yes. Sorry I misunderstood your question.
> UBIFS readdir() address this already, if you ask it to readdir()
> from pos X and X is not present it will jump to the next best entry X'.
> UBIFS does so since ever.

OK, good.  So the random nonce is stored with the entry, and the hash
you can always recalculate from the filename, so if you return entries
in nonce+hash order, then you can keep that order stable even across
deletes and renames.

--b.

Re: [PATCH 3/6] ubifs: Use 64bit readdir cookies

[Richard Weinberger]

On 29.12.2016 17:59, J. Bruce Fields wrote:
> OK, good.  So the random nonce is stored with the entry, and the hash
> you can always recalculate from the filename, so if you return entries
> in nonce+hash order, then you can keep that order stable even across
> deletes and renames.

Yes.

Thanks,
//richard

Re: [PATCH 3/6] ubifs: Use 64bit readdir cookies

[J. Bruce Fields]

On Thu, Dec 29, 2016 at 06:05:54PM +0100, Richard Weinberger wrote:
> On 29.12.2016 17:59, J. Bruce Fields wrote:
> > OK, good.  So the random nonce is stored with the entry, and the hash
> > you can always recalculate from the filename, so if you return entries
> > in nonce+hash order, then you can keep that order stable even across
> > deletes and renames.
> 
> Yes.

Thanks, feel free to add a

	Reviewed-by: J. Bruce Fields <bfields@redhat.com>

to this and 6/6.

--b.

[PATCH 4/6] ubifs: Maintain a parent pointer

[Richard Weinberger]

The new feature UBIFS_FLG_PARENTPOINTER allows looking
up the parent. Usually the Linux VFS walks down the filesystem
and no parent pointers are needed. But when a filesystem
is exportable via NFS such a lookup is needed.
We can use a padding field in struct ubifs_ino_node to
maintain a pointer to the parent inode.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/ubifs/dir.c         | 21 ++++++++++++++++++++-
 fs/ubifs/journal.c     |  5 ++++-
 fs/ubifs/sb.c          |  2 ++
 fs/ubifs/super.c       |  1 +
 fs/ubifs/ubifs-media.h | 12 +++++++++---
 fs/ubifs/ubifs.h       |  4 ++++
 6 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 3b8c08dad75b..5485d836af21 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -171,6 +171,7 @@ struct inode *ubifs_new_inode(struct ubifs_info *c, struct inode *dir,
 	}
 
 	inode->i_ino = ++c->highest_inum;
+	ui->parent_inum = inode->i_ino;
 	/*
 	 * The creation sequence number remains with this inode for its
 	 * lifetime. All nodes for this inode have a greater sequence number,
@@ -1409,7 +1410,7 @@ static int do_rename(struct inode *old_dir, struct dentry *old_dentry,
 	if (unlink)
 		ubifs_assert(inode_is_locked(new_inode));
 
-	if (old_dir != new_dir) {
+	if (move) {
 		if (ubifs_crypt_is_encrypted(new_dir) &&
 		    !fscrypt_has_permitted_context(new_dir, old_inode))
 			return -EPERM;
@@ -1563,8 +1564,12 @@ static int do_rename(struct inode *old_dir, struct dentry *old_dentry,
 		mark_inode_dirty(whiteout);
 		whiteout->i_state &= ~I_LINKABLE;
 		iput(whiteout);
+		whiteout_ui->parent_inum = new_dir->i_ino;
 	}
 
+	if (move)
+		old_inode_ui->parent_inum = new_dir->i_ino;
+
 	err = ubifs_jnl_rename(c, old_dir, old_inode, &old_nm, new_dir,
 			       new_inode, &new_nm, whiteout, sync);
 	if (err)
@@ -1606,6 +1611,8 @@ static int do_rename(struct inode *old_dir, struct dentry *old_dentry,
 				inc_nlink(old_dir);
 		}
 	}
+	if (move)
+		old_inode_ui->parent_inum = old_dir->i_ino;
 	if (whiteout) {
 		drop_nlink(whiteout);
 		iput(whiteout);
@@ -1627,6 +1634,8 @@ static int ubifs_xrename(struct inode *old_dir, struct dentry *old_dentry,
 	int sync = IS_DIRSYNC(old_dir) || IS_DIRSYNC(new_dir);
 	struct inode *fst_inode = d_inode(old_dentry);
 	struct inode *snd_inode = d_inode(new_dentry);
+	struct ubifs_inode *fst_inode_ui = ubifs_inode(fst_inode);
+	struct ubifs_inode *snd_inode_ui = ubifs_inode(snd_inode);
 	struct timespec time;
 	int err;
 	struct fscrypt_name fst_nm, snd_nm;
@@ -1658,6 +1667,11 @@ static int ubifs_xrename(struct inode *old_dir, struct dentry *old_dentry,
 	old_dir->i_mtime = old_dir->i_ctime = time;
 	new_dir->i_mtime = new_dir->i_ctime = time;
 
+	if (new_dir != old_dir) {
+		fst_inode_ui->parent_inum = new_dir->i_ino;
+		snd_inode_ui->parent_inum = old_dir->i_ino;
+	}
+
 	if (old_dir != new_dir) {
 		if (S_ISDIR(fst_inode->i_mode) && !S_ISDIR(snd_inode->i_mode)) {
 			inc_nlink(new_dir);
@@ -1672,6 +1686,11 @@ static int ubifs_xrename(struct inode *old_dir, struct dentry *old_dentry,
 	err = ubifs_jnl_xrename(c, old_dir, fst_inode, &fst_nm, new_dir,
 				snd_inode, &snd_nm, sync);
 
+	if (err && new_dir != old_dir) {
+		fst_inode_ui->parent_inum = old_dir->i_ino;
+		snd_inode_ui->parent_inum = new_dir->i_ino;
+	}
+
 	unlock_4_inodes(old_dir, new_dir, NULL, NULL);
 	ubifs_release_budget(c, &req);
 
diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
index a459211a1c21..4a76c14fb07c 100644
--- a/fs/ubifs/journal.c
+++ b/fs/ubifs/journal.c
@@ -66,7 +66,6 @@
  */
 static inline void zero_ino_node_unused(struct ubifs_ino_node *ino)
 {
-	memset(ino->padding1, 0, 4);
 	memset(ino->padding2, 0, 26);
 }
 
@@ -470,6 +469,10 @@ static void pack_inode(struct ubifs_info *c, struct ubifs_ino_node *ino,
 	ino->xattr_cnt   = cpu_to_le32(ui->xattr_cnt);
 	ino->xattr_size  = cpu_to_le32(ui->xattr_size);
 	ino->xattr_names = cpu_to_le32(ui->xattr_names);
+	if (c->parent_pointer)
+		ino->parent_inum = cpu_to_le32(ui->parent_inum);
+	else
+		ino->parent_inum = 0;
 	zero_ino_node_unused(ino);
 
 	/*
diff --git a/fs/ubifs/sb.c b/fs/ubifs/sb.c
index 7f1ead29e727..f012cd411382 100644
--- a/fs/ubifs/sb.c
+++ b/fs/ubifs/sb.c
@@ -164,6 +164,7 @@ static int create_default_filesystem(struct ubifs_info *c)
 	if (big_lpt)
 		sup_flags |= UBIFS_FLG_BIGLPT;
 	sup_flags |= UBIFS_FLG_DOUBLE_HASH;
+	sup_flags |= UBIFS_FLG_PARENTPOINTER;
 
 	sup->ch.node_type  = UBIFS_SB_NODE;
 	sup->key_hash      = UBIFS_KEY_HASH_R5;
@@ -633,6 +634,7 @@ int ubifs_read_superblock(struct ubifs_info *c)
 	c->space_fixup = !!(sup_flags & UBIFS_FLG_SPACE_FIXUP);
 	c->double_hash = !!(sup_flags & UBIFS_FLG_DOUBLE_HASH);
 	c->encrypted = !!(sup_flags & UBIFS_FLG_ENCRYPTION);
+	c->parent_pointer = !!(sup_flags & UBIFS_FLG_PARENTPOINTER);
 
 	if ((sup_flags & ~UBIFS_FLG_MASK) != 0) {
 		ubifs_err(c, "Unknown feature flags found: %#x",
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index e08aa04fc835..c50952f43e36 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -154,6 +154,7 @@ struct inode *ubifs_iget(struct super_block *sb, unsigned long inum)
 	ui->synced_i_size = ui->ui_size = inode->i_size;
 
 	ui->xattr = (ui->flags & UBIFS_XATTR_FL) ? 1 : 0;
+	ui->parent_inum = le32_to_cpu(ino->parent_inum);
 
 	err = validate_inode(c, inode);
 	if (err)
diff --git a/fs/ubifs/ubifs-media.h b/fs/ubifs/ubifs-media.h
index 5939776c7359..b3cb76cedf20 100644
--- a/fs/ubifs/ubifs-media.h
+++ b/fs/ubifs/ubifs-media.h
@@ -427,15 +427,21 @@ enum {
  * UBIFS_FLG_DOUBLE_HASH: store a 32bit cookie in directory entry nodes to
  *			  support 64bit cookies for lookups by hash
  * UBIFS_FLG_ENCRYPTION: this filesystem contains encrypted files
+ * UBIFS_FLG_PARENTPOINTER: inode nodes maintain a pointer to the parent dir
  */
 enum {
 	UBIFS_FLG_BIGLPT = 0x02,
 	UBIFS_FLG_SPACE_FIXUP = 0x04,
 	UBIFS_FLG_DOUBLE_HASH = 0x08,
 	UBIFS_FLG_ENCRYPTION = 0x10,
+	UBIFS_FLG_PARENTPOINTER = 0x20,
 };
 
-#define UBIFS_FLG_MASK (UBIFS_FLG_BIGLPT|UBIFS_FLG_SPACE_FIXUP|UBIFS_FLG_DOUBLE_HASH|UBIFS_FLG_ENCRYPTION)
+#define UBIFS_FLG_MASK (UBIFS_FLG_BIGLPT		\
+			|UBIFS_FLG_SPACE_FIXUP		\
+			|UBIFS_FLG_DOUBLE_HASH		\
+			|UBIFS_FLG_ENCRYPTION		\
+			|UBIFS_FLG_PARENTPOINTER)
 
 /**
  * struct ubifs_ch - common header node.
@@ -494,7 +500,7 @@ union ubifs_dev_desc {
  * @data_len: inode data length
  * @xattr_cnt: count of extended attributes this inode has
  * @xattr_size: summarized size of all extended attributes in bytes
- * @padding1: reserved for future, zeroes
+ * @parent_inum: parent inode number
  * @xattr_names: sum of lengths of all extended attribute names belonging to
  *               this inode
  * @compr_type: compression type used for this inode
@@ -528,7 +534,7 @@ struct ubifs_ino_node {
 	__le32 data_len;
 	__le32 xattr_cnt;
 	__le32 xattr_size;
-	__u8 padding1[4]; /* Watch 'zero_ino_node_unused()' if changing! */
+	__le32 parent_inum;
 	__le32 xattr_names;
 	__le16 compr_type;
 	__u8 padding2[26]; /* Watch 'zero_ino_node_unused()' if changing! */
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index 0532a6f82b1d..e1b7531650af 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -353,6 +353,7 @@ struct ubifs_gced_idx_leb {
  *                 currently stored on the flash; used only for regular file
  *                 inodes
  * @ui_size: inode size used by UBIFS when writing to flash
+ * @parent_inum: inode number of the parent directory
  * @flags: inode flags (@UBIFS_COMPR_FL, etc)
  * @compr_type: default compression type used for this inode
  * @last_page_read: page number of last page read (for bulk read)
@@ -404,6 +405,7 @@ struct ubifs_inode {
 	spinlock_t ui_lock;
 	loff_t synced_i_size;
 	loff_t ui_size;
+	ino_t parent_inum;
 	int flags;
 	pgoff_t last_page_read;
 	pgoff_t read_in_a_row;
@@ -1018,6 +1020,7 @@ struct ubifs_debug_info;
  * @space_fixup: flag indicating that free space in LEBs needs to be cleaned up
  * @double_hash: flag indicating that we can do lookups by hash
  * @encrypted: flag indicating that this file system contains encrypted files
+ * @parent_pointer: flag indicating that inodes have pointers to the parent dir
  * @no_chk_data_crc: do not check CRCs when reading data nodes (except during
  *                   recovery)
  * @bulk_read: enable bulk-reads
@@ -1262,6 +1265,7 @@ struct ubifs_info {
 	unsigned int space_fixup:1;
 	unsigned int double_hash:1;
 	unsigned int encrypted:1;
+	unsigned int parent_pointer:1;
 	unsigned int no_chk_data_crc:1;
 	unsigned int bulk_read:1;
 	unsigned int default_compr:2;
-- 
2.7.3

Re: [PATCH 4/6] ubifs: Maintain a parent pointer

[Marcus Folkesson]

On Thu, Dec 01, 2016 at 11:02:19PM +0100, Richard Weinberger wrote:
>  
> +	if (new_dir != old_dir) {
> +		fst_inode_ui->parent_inum = new_dir->i_ino;
> +		snd_inode_ui->parent_inum = old_dir->i_ino;
> +	}
> +
>  	if (old_dir != new_dir) {
>  		if (S_ISDIR(fst_inode->i_mode) && !S_ISDIR(snd_inode->i_mode)) {
>  			inc_nlink(new_dir);

Insignificant thing, but use the existing if-statment instead?

Cheers,
Marcus Folkesson

Re: [PATCH 4/6] ubifs: Maintain a parent pointer

[Richard Weinberger]

On 02.12.2016 10:28, Marcus Folkesson wrote:
> On Thu, Dec 01, 2016 at 11:02:19PM +0100, Richard Weinberger wrote:
>>  
>> +	if (new_dir != old_dir) {
>> +		fst_inode_ui->parent_inum = new_dir->i_ino;
>> +		snd_inode_ui->parent_inum = old_dir->i_ino;
>> +	}
>> +
>>  	if (old_dir != new_dir) {
>>  		if (S_ISDIR(fst_inode->i_mode) && !S_ISDIR(snd_inode->i_mode)) {
>>  			inc_nlink(new_dir);
> 
> Insignificant thing, but use the existing if-statment instead?

Of course.
I moved the line around and oversaw the existing branch. :)

Thanks,
//richard

Re: [PATCH 4/6] ubifs: Maintain a parent pointer

[Hyunchul Lee]

Hi Richard

I found a mistake in this patch.

On Thu, Dec 01, 2016 at 11:02:19PM +0100, Richard Weinberger wrote:
> The new feature UBIFS_FLG_PARENTPOINTER allows looking
> up the parent. Usually the Linux VFS walks down the filesystem
> and no parent pointers are needed. But when a filesystem
> is exportable via NFS such a lookup is needed.
> We can use a padding field in struct ubifs_ino_node to
> maintain a pointer to the parent inode.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  fs/ubifs/dir.c         | 21 ++++++++++++++++++++-
>  fs/ubifs/journal.c     |  5 ++++-
>  fs/ubifs/sb.c          |  2 ++
>  fs/ubifs/super.c       |  1 +
>  fs/ubifs/ubifs-media.h | 12 +++++++++---
>  fs/ubifs/ubifs.h       |  4 ++++
>  6 files changed, 40 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> index 3b8c08dad75b..5485d836af21 100644
> --- a/fs/ubifs/dir.c
> +++ b/fs/ubifs/dir.c
> @@ -171,6 +171,7 @@ struct inode *ubifs_new_inode(struct ubifs_info *c, struct inode *dir,
>  	}
>  
>  	inode->i_ino = ++c->highest_inum;
> +	ui->parent_inum = inode->i_ino;

I guess that dir->i_ino should be assigned to ui->parent_inum.

>  	/*
>  	 * The creation sequence number remains with this inode for its
>  	 * lifetime. All nodes for this inode have a greater sequence number,
> @@ -1409,7 +1410,7 @@ static int do_rename(struct inode *old_dir, struct dentry *old_dentry,
>  	if (unlink)
>  		ubifs_assert(inode_is_locked(new_inode));
>  
> -	if (old_dir != new_dir) {
> +	if (move) {
>  		if (ubifs_crypt_is_encrypted(new_dir) &&
>  		    !fscrypt_has_permitted_context(new_dir, old_inode))
>  			return -EPERM;
> @@ -1563,8 +1564,12 @@ static int do_rename(struct inode *old_dir, struct dentry *old_dentry,
>  		mark_inode_dirty(whiteout);
>  		whiteout->i_state &= ~I_LINKABLE;
>  		iput(whiteout);
> +		whiteout_ui->parent_inum = new_dir->i_ino;
>  	}
>  
> +	if (move)
> +		old_inode_ui->parent_inum = new_dir->i_ino;
> +
>  	err = ubifs_jnl_rename(c, old_dir, old_inode, &old_nm, new_dir,
>  			       new_inode, &new_nm, whiteout, sync);
>  	if (err)
> @@ -1606,6 +1611,8 @@ static int do_rename(struct inode *old_dir, struct dentry *old_dentry,
>  				inc_nlink(old_dir);
>  		}
>  	}
> +	if (move)
> +		old_inode_ui->parent_inum = old_dir->i_ino;
>  	if (whiteout) {
>  		drop_nlink(whiteout);
>  		iput(whiteout);
> @@ -1627,6 +1634,8 @@ static int ubifs_xrename(struct inode *old_dir, struct dentry *old_dentry,
>  	int sync = IS_DIRSYNC(old_dir) || IS_DIRSYNC(new_dir);
>  	struct inode *fst_inode = d_inode(old_dentry);
>  	struct inode *snd_inode = d_inode(new_dentry);
> +	struct ubifs_inode *fst_inode_ui = ubifs_inode(fst_inode);
> +	struct ubifs_inode *snd_inode_ui = ubifs_inode(snd_inode);
>  	struct timespec time;
>  	int err;
>  	struct fscrypt_name fst_nm, snd_nm;
> @@ -1658,6 +1667,11 @@ static int ubifs_xrename(struct inode *old_dir, struct dentry *old_dentry,
>  	old_dir->i_mtime = old_dir->i_ctime = time;
>  	new_dir->i_mtime = new_dir->i_ctime = time;
>  
> +	if (new_dir != old_dir) {
> +		fst_inode_ui->parent_inum = new_dir->i_ino;
> +		snd_inode_ui->parent_inum = old_dir->i_ino;
> +	}
> +
>  	if (old_dir != new_dir) {
>  		if (S_ISDIR(fst_inode->i_mode) && !S_ISDIR(snd_inode->i_mode)) {
>  			inc_nlink(new_dir);
> @@ -1672,6 +1686,11 @@ static int ubifs_xrename(struct inode *old_dir, struct dentry *old_dentry,
>  	err = ubifs_jnl_xrename(c, old_dir, fst_inode, &fst_nm, new_dir,
>  				snd_inode, &snd_nm, sync);
>  
> +	if (err && new_dir != old_dir) {
> +		fst_inode_ui->parent_inum = old_dir->i_ino;
> +		snd_inode_ui->parent_inum = new_dir->i_ino;
> +	}
> +
>  	unlock_4_inodes(old_dir, new_dir, NULL, NULL);
>  	ubifs_release_budget(c, &req);
>  
> diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
> index a459211a1c21..4a76c14fb07c 100644
> --- a/fs/ubifs/journal.c
> +++ b/fs/ubifs/journal.c
> @@ -66,7 +66,6 @@
>   */
>  static inline void zero_ino_node_unused(struct ubifs_ino_node *ino)
>  {
> -	memset(ino->padding1, 0, 4);
>  	memset(ino->padding2, 0, 26);
>  }
>  
> @@ -470,6 +469,10 @@ static void pack_inode(struct ubifs_info *c, struct ubifs_ino_node *ino,
>  	ino->xattr_cnt   = cpu_to_le32(ui->xattr_cnt);
>  	ino->xattr_size  = cpu_to_le32(ui->xattr_size);
>  	ino->xattr_names = cpu_to_le32(ui->xattr_names);
> +	if (c->parent_pointer)
> +		ino->parent_inum = cpu_to_le32(ui->parent_inum);
> +	else
> +		ino->parent_inum = 0;
>  	zero_ino_node_unused(ino);
>  
>  	/*
> diff --git a/fs/ubifs/sb.c b/fs/ubifs/sb.c
> index 7f1ead29e727..f012cd411382 100644
> --- a/fs/ubifs/sb.c
> +++ b/fs/ubifs/sb.c
> @@ -164,6 +164,7 @@ static int create_default_filesystem(struct ubifs_info *c)
>  	if (big_lpt)
>  		sup_flags |= UBIFS_FLG_BIGLPT;
>  	sup_flags |= UBIFS_FLG_DOUBLE_HASH;
> +	sup_flags |= UBIFS_FLG_PARENTPOINTER;
>  
>  	sup->ch.node_type  = UBIFS_SB_NODE;
>  	sup->key_hash      = UBIFS_KEY_HASH_R5;
> @@ -633,6 +634,7 @@ int ubifs_read_superblock(struct ubifs_info *c)
>  	c->space_fixup = !!(sup_flags & UBIFS_FLG_SPACE_FIXUP);
>  	c->double_hash = !!(sup_flags & UBIFS_FLG_DOUBLE_HASH);
>  	c->encrypted = !!(sup_flags & UBIFS_FLG_ENCRYPTION);
> +	c->parent_pointer = !!(sup_flags & UBIFS_FLG_PARENTPOINTER);
>  
>  	if ((sup_flags & ~UBIFS_FLG_MASK) != 0) {
>  		ubifs_err(c, "Unknown feature flags found: %#x",
> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index e08aa04fc835..c50952f43e36 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
> @@ -154,6 +154,7 @@ struct inode *ubifs_iget(struct super_block *sb, unsigned long inum)
>  	ui->synced_i_size = ui->ui_size = inode->i_size;
>  
>  	ui->xattr = (ui->flags & UBIFS_XATTR_FL) ? 1 : 0;
> +	ui->parent_inum = le32_to_cpu(ino->parent_inum);
>  
>  	err = validate_inode(c, inode);
>  	if (err)
> diff --git a/fs/ubifs/ubifs-media.h b/fs/ubifs/ubifs-media.h
> index 5939776c7359..b3cb76cedf20 100644
> --- a/fs/ubifs/ubifs-media.h
> +++ b/fs/ubifs/ubifs-media.h
> @@ -427,15 +427,21 @@ enum {
>   * UBIFS_FLG_DOUBLE_HASH: store a 32bit cookie in directory entry nodes to
>   *			  support 64bit cookies for lookups by hash
>   * UBIFS_FLG_ENCRYPTION: this filesystem contains encrypted files
> + * UBIFS_FLG_PARENTPOINTER: inode nodes maintain a pointer to the parent dir
>   */
>  enum {
>  	UBIFS_FLG_BIGLPT = 0x02,
>  	UBIFS_FLG_SPACE_FIXUP = 0x04,
>  	UBIFS_FLG_DOUBLE_HASH = 0x08,
>  	UBIFS_FLG_ENCRYPTION = 0x10,
> +	UBIFS_FLG_PARENTPOINTER = 0x20,
>  };
>  
> -#define UBIFS_FLG_MASK (UBIFS_FLG_BIGLPT|UBIFS_FLG_SPACE_FIXUP|UBIFS_FLG_DOUBLE_HASH|UBIFS_FLG_ENCRYPTION)
> +#define UBIFS_FLG_MASK (UBIFS_FLG_BIGLPT		\
> +			|UBIFS_FLG_SPACE_FIXUP		\
> +			|UBIFS_FLG_DOUBLE_HASH		\
> +			|UBIFS_FLG_ENCRYPTION		\
> +			|UBIFS_FLG_PARENTPOINTER)
>  
>  /**
>   * struct ubifs_ch - common header node.
> @@ -494,7 +500,7 @@ union ubifs_dev_desc {
>   * @data_len: inode data length
>   * @xattr_cnt: count of extended attributes this inode has
>   * @xattr_size: summarized size of all extended attributes in bytes
> - * @padding1: reserved for future, zeroes
> + * @parent_inum: parent inode number
>   * @xattr_names: sum of lengths of all extended attribute names belonging to
>   *               this inode
>   * @compr_type: compression type used for this inode
> @@ -528,7 +534,7 @@ struct ubifs_ino_node {
>  	__le32 data_len;
>  	__le32 xattr_cnt;
>  	__le32 xattr_size;
> -	__u8 padding1[4]; /* Watch 'zero_ino_node_unused()' if changing! */
> +	__le32 parent_inum;
>  	__le32 xattr_names;
>  	__le16 compr_type;
>  	__u8 padding2[26]; /* Watch 'zero_ino_node_unused()' if changing! */
> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
> index 0532a6f82b1d..e1b7531650af 100644
> --- a/fs/ubifs/ubifs.h
> +++ b/fs/ubifs/ubifs.h
> @@ -353,6 +353,7 @@ struct ubifs_gced_idx_leb {
>   *                 currently stored on the flash; used only for regular file
>   *                 inodes
>   * @ui_size: inode size used by UBIFS when writing to flash
> + * @parent_inum: inode number of the parent directory
>   * @flags: inode flags (@UBIFS_COMPR_FL, etc)
>   * @compr_type: default compression type used for this inode
>   * @last_page_read: page number of last page read (for bulk read)
> @@ -404,6 +405,7 @@ struct ubifs_inode {
>  	spinlock_t ui_lock;
>  	loff_t synced_i_size;
>  	loff_t ui_size;
> +	ino_t parent_inum;
>  	int flags;
>  	pgoff_t last_page_read;
>  	pgoff_t read_in_a_row;
> @@ -1018,6 +1020,7 @@ struct ubifs_debug_info;
>   * @space_fixup: flag indicating that free space in LEBs needs to be cleaned up
>   * @double_hash: flag indicating that we can do lookups by hash
>   * @encrypted: flag indicating that this file system contains encrypted files
> + * @parent_pointer: flag indicating that inodes have pointers to the parent dir
>   * @no_chk_data_crc: do not check CRCs when reading data nodes (except during
>   *                   recovery)
>   * @bulk_read: enable bulk-reads
> @@ -1262,6 +1265,7 @@ struct ubifs_info {
>  	unsigned int space_fixup:1;
>  	unsigned int double_hash:1;
>  	unsigned int encrypted:1;
> +	unsigned int parent_pointer:1;
>  	unsigned int no_chk_data_crc:1;
>  	unsigned int bulk_read:1;
>  	unsigned int default_compr:2;
> -- 
> 2.7.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

Thanks,
Hyunchul

Re: [PATCH 4/6] ubifs: Maintain a parent pointer

[Richard Weinberger]

Hyunchul,

Am 28.04.2017 um 10:31 schrieb Hyunchul Lee:
> I guess that dir->i_ino should be assigned to ui->parent_inum.

Uff, right.
It is obvious that I did this series in a hurry. :D

Thanks,
//richard

[PATCH 5/6] ubifs: Implement export_operations

[Richard Weinberger]

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/ubifs/super.c | 31 +++++++++++++++++++++++++++++++
 fs/ubifs/ubifs.h |  1 +
 2 files changed, 32 insertions(+)

diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index c50952f43e36..be5b697d8214 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -2006,6 +2006,37 @@ struct fscrypt_operations ubifs_crypt_operations = {
 };
 #endif
 
+static struct dentry *ubifs_get_parent(struct dentry *child)
+{
+	struct ubifs_inode *ui = ubifs_inode(d_inode(child));
+
+        return d_obtain_alias(ubifs_iget(child->d_sb, ui->parent_inum));
+}
+
+static struct inode *ubifs_nfs_get_inode(struct super_block *sb, uint64_t ino,
+					 uint32_t generation)
+{
+	return ubifs_iget(sb, ino);
+}
+
+static struct dentry *ubifs_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, ubifs_nfs_get_inode);
+}
+
+static struct dentry *ubifs_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, ubifs_nfs_get_inode);
+}
+
+static const struct export_operations ubifs_export_ops = {
+	.get_parent = ubifs_get_parent,
+	.fh_to_dentry = ubifs_fh_to_dentry,
+	.fh_to_parent = ubifs_fh_to_parent,
+};
+
 static int ubifs_fill_super(struct super_block *sb, void *data, int silent)
 {
 	struct ubifs_info *c = sb->s_fs_info;
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index e1b7531650af..8b0ac0a8afea 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -41,6 +41,7 @@
 #include <linux/fscrypto.h>
 #include <linux/random.h>
 #include <linux/compat.h>
+#include <linux/exportfs.h>
 #include "ubifs-media.h"
 
 /* Version of this UBIFS implementation */
-- 
2.7.3

[PATCH 6/6] ubifs: Wire up NFS support

[Richard Weinberger]

Since we have 64bit readdir cookies and export operations
we can finally enable NFS export support for UBIFS.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/ubifs/dir.c   | 9 ++-------
 fs/ubifs/super.c | 3 +++
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 5485d836af21..13d13afd2976 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -289,11 +289,8 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry,
 done:
 	kfree(dent);
 	fscrypt_free_filename(&nm);
-	/*
-	 * Note, d_splice_alias() would be required instead if we supported
-	 * NFS.
-	 */
-	d_add(dentry, inode);
+
+	d_splice_alias(inode, dentry);
 	return NULL;
 
 out_dent:
@@ -524,8 +521,6 @@ static unsigned int vfs_dent_type(uint8_t type)
  * properly by means of saving full directory entry name in the private field
  * of the file description object.
  *
- * This means that UBIFS cannot support NFS which requires full
- * 'seekdir()'/'telldir()' support.
  */
 static int ubifs_readdir(struct file *file, struct dir_context *ctx)
 {
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index be5b697d8214..4cb7f641f35c 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -2092,6 +2092,9 @@ static int ubifs_fill_super(struct super_block *sb, void *data, int silent)
 		goto out_unlock;
 	}
 
+	if (c->parent_pointer && c->double_hash)
+		sb->s_export_op = &ubifs_export_ops;
+
 	/* Read the root inode */
 	root = ubifs_iget(sb, UBIFS_ROOT_INO);
 	if (IS_ERR(root)) {
-- 
2.7.3

Re: [PATCH 6/6] ubifs: Wire up NFS support

[J. Bruce Fields]

On Thu, Dec 01, 2016 at 11:02:21PM +0100, Richard Weinberger wrote:
> Since we have 64bit readdir cookies and export operations
> we can finally enable NFS export support for UBIFS.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  fs/ubifs/dir.c   | 9 ++-------
>  fs/ubifs/super.c | 3 +++
>  2 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> index 5485d836af21..13d13afd2976 100644
> --- a/fs/ubifs/dir.c
> +++ b/fs/ubifs/dir.c
> @@ -289,11 +289,8 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry,
>  done:
>  	kfree(dent);
>  	fscrypt_free_filename(&nm);
> -	/*
> -	 * Note, d_splice_alias() would be required instead if we supported
> -	 * NFS.
> -	 */
> -	d_add(dentry, inode);
> +
> +	d_splice_alias(inode, dentry);
>  	return NULL;

I'm pretty sure that should be 

	return d_splice_alias(inode, dentry);

--b.

>  
>  out_dent:
> @@ -524,8 +521,6 @@ static unsigned int vfs_dent_type(uint8_t type)
>   * properly by means of saving full directory entry name in the private field
>   * of the file description object.
>   *
> - * This means that UBIFS cannot support NFS which requires full
> - * 'seekdir()'/'telldir()' support.
>   */
>  static int ubifs_readdir(struct file *file, struct dir_context *ctx)
>  {
> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index be5b697d8214..4cb7f641f35c 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
> @@ -2092,6 +2092,9 @@ static int ubifs_fill_super(struct super_block *sb, void *data, int silent)
>  		goto out_unlock;
>  	}
>  
> +	if (c->parent_pointer && c->double_hash)
> +		sb->s_export_op = &ubifs_export_ops;
> +
>  	/* Read the root inode */
>  	root = ubifs_iget(sb, UBIFS_ROOT_INO);
>  	if (IS_ERR(root)) {
> -- 
> 2.7.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] ubifs: Wire up NFS support

[Richard Weinberger]

Bruce,

On 29.12.2016 03:56, J. Bruce Fields wrote:
>> -	 * Note, d_splice_alias() would be required instead if we supported
>> -	 * NFS.
>> -	 */
>> -	d_add(dentry, inode);
>> +
>> +	d_splice_alias(inode, dentry);
>>  	return NULL;
> 
> I'm pretty sure that should be 
> 
> 	return d_splice_alias(inode, dentry);

You are right.
/me assumed d_splice_alias() is consistent with d_add(). :-(

Thanks,
//richard

[PATCH 0/6] UBIFS NFS export support v2

[Richard Weinberger]

Adding fscrypto support to UBIFS forced us to implement 64bit
cookies. Using this building block we can finally implement
export operations and allow UBIFS being exported via NFS.
This series adds the last missing bits.

It can be obtained from:
git://git.infradead.org/users/rw/linux.git ubifs_export_v2

Changes since v1, https://lkml.org/lkml/2016/12/1/711
 - Addressed comments from Marcus Folkesson, Hyunchul Lee and J. Bruce Fields
 - Added Acks/Reviewed-bys from J. Bruce Fields and Andreas Dilger

Richard Weinberger (6):
  ext4: Move is_32bit_api() to generic code
  ubifs: Provide a custom llseek for directories
  ubifs: Use 64bit readdir cookies
  ubifs: Maintain a parent pointer
  ubifs: Implement export_operations
  ubifs: Wire up NFS support

 fs/ext4/dir.c          |  9 -----
 fs/ubifs/dir.c         | 89 +++++++++++++++++++++++++++++++++++++-------------
 fs/ubifs/journal.c     |  5 ++-
 fs/ubifs/key.h         | 61 ++++++++++++++++++++++++++++++++++
 fs/ubifs/sb.c          |  2 ++
 fs/ubifs/super.c       | 39 ++++++++++++++++++++++
 fs/ubifs/ubifs-media.h | 18 ++++++++--
 fs/ubifs/ubifs.h       |  6 ++++
 include/linux/compat.h |  2 ++
 9 files changed, 195 insertions(+), 36 deletions(-)

-- 
2.12.0

[PATCH 4/6] ubifs: Maintain a parent pointer

[Richard Weinberger]

The new feature UBIFS_FLG_PARENTPOINTER allows looking
up the parent. Usually the Linux VFS walks down the filesystem
and no parent pointers are needed. But when a filesystem
is exportable via NFS such a lookup is needed.
We can use a padding field in struct ubifs_ino_node to
maintain a pointer to the parent inode.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/ubifs/dir.c         | 21 +++++++++++++++++++--
 fs/ubifs/journal.c     |  5 ++++-
 fs/ubifs/sb.c          |  2 ++
 fs/ubifs/super.c       |  1 +
 fs/ubifs/ubifs-media.h | 12 +++++++++---
 fs/ubifs/ubifs.h       |  4 ++++
 6 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index e79b529df9c3..a6eadb52a1a8 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -171,6 +171,7 @@ struct inode *ubifs_new_inode(struct ubifs_info *c, struct inode *dir,
 	}
 
 	inode->i_ino = ++c->highest_inum;
+	ui->parent_inum = dir->i_ino;
 	/*
 	 * The creation sequence number remains with this inode for its
 	 * lifetime. All nodes for this inode have a greater sequence number,
@@ -1374,7 +1375,7 @@ static int do_rename(struct inode *old_dir, struct dentry *old_dentry,
 	if (unlink)
 		ubifs_assert(inode_is_locked(new_inode));
 
-	if (old_dir != new_dir) {
+	if (move) {
 		if (ubifs_crypt_is_encrypted(new_dir) &&
 		    !fscrypt_has_permitted_context(new_dir, old_inode))
 			return -EPERM;
@@ -1528,8 +1529,12 @@ static int do_rename(struct inode *old_dir, struct dentry *old_dentry,
 		mark_inode_dirty(whiteout);
 		whiteout->i_state &= ~I_LINKABLE;
 		iput(whiteout);
+		whiteout_ui->parent_inum = new_dir->i_ino;
 	}
 
+	if (move)
+		old_inode_ui->parent_inum = new_dir->i_ino;
+
 	err = ubifs_jnl_rename(c, old_dir, old_inode, &old_nm, new_dir,
 			       new_inode, &new_nm, whiteout, sync);
 	if (err)
@@ -1571,6 +1576,8 @@ static int do_rename(struct inode *old_dir, struct dentry *old_dentry,
 				inc_nlink(old_dir);
 		}
 	}
+	if (move)
+		old_inode_ui->parent_inum = old_dir->i_ino;
 	if (whiteout) {
 		drop_nlink(whiteout);
 		iput(whiteout);
@@ -1592,6 +1599,8 @@ static int ubifs_xrename(struct inode *old_dir, struct dentry *old_dentry,
 	int sync = IS_DIRSYNC(old_dir) || IS_DIRSYNC(new_dir);
 	struct inode *fst_inode = d_inode(old_dentry);
 	struct inode *snd_inode = d_inode(new_dentry);
+	struct ubifs_inode *fst_inode_ui = ubifs_inode(fst_inode);
+	struct ubifs_inode *snd_inode_ui = ubifs_inode(snd_inode);
 	struct timespec time;
 	int err;
 	struct fscrypt_name fst_nm, snd_nm;
@@ -1623,7 +1632,10 @@ static int ubifs_xrename(struct inode *old_dir, struct dentry *old_dentry,
 	old_dir->i_mtime = old_dir->i_ctime = time;
 	new_dir->i_mtime = new_dir->i_ctime = time;
 
-	if (old_dir != new_dir) {
+	if (new_dir != old_dir) {
+		fst_inode_ui->parent_inum = new_dir->i_ino;
+		snd_inode_ui->parent_inum = old_dir->i_ino;
+
 		if (S_ISDIR(fst_inode->i_mode) && !S_ISDIR(snd_inode->i_mode)) {
 			inc_nlink(new_dir);
 			drop_nlink(old_dir);
@@ -1637,6 +1649,11 @@ static int ubifs_xrename(struct inode *old_dir, struct dentry *old_dentry,
 	err = ubifs_jnl_xrename(c, old_dir, fst_inode, &fst_nm, new_dir,
 				snd_inode, &snd_nm, sync);
 
+	if (err && new_dir != old_dir) {
+		fst_inode_ui->parent_inum = old_dir->i_ino;
+		snd_inode_ui->parent_inum = new_dir->i_ino;
+	}
+
 	unlock_4_inodes(old_dir, new_dir, NULL, NULL);
 	ubifs_release_budget(c, &req);
 
diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
index 294519b98874..8eaf8f2f1fe1 100644
--- a/fs/ubifs/journal.c
+++ b/fs/ubifs/journal.c
@@ -66,7 +66,6 @@
  */
 static inline void zero_ino_node_unused(struct ubifs_ino_node *ino)
 {
-	memset(ino->padding1, 0, 4);
 	memset(ino->padding2, 0, 26);
 }
 
@@ -470,6 +469,10 @@ static void pack_inode(struct ubifs_info *c, struct ubifs_ino_node *ino,
 	ino->xattr_cnt   = cpu_to_le32(ui->xattr_cnt);
 	ino->xattr_size  = cpu_to_le32(ui->xattr_size);
 	ino->xattr_names = cpu_to_le32(ui->xattr_names);
+	if (c->parent_pointer)
+		ino->parent_inum = cpu_to_le32(ui->parent_inum);
+	else
+		ino->parent_inum = 0;
 	zero_ino_node_unused(ino);
 
 	/*
diff --git a/fs/ubifs/sb.c b/fs/ubifs/sb.c
index 8c25081a5109..f2097c47f629 100644
--- a/fs/ubifs/sb.c
+++ b/fs/ubifs/sb.c
@@ -166,6 +166,7 @@ static int create_default_filesystem(struct ubifs_info *c)
 	if (big_lpt)
 		sup_flags |= UBIFS_FLG_BIGLPT;
 	sup_flags |= UBIFS_FLG_DOUBLE_HASH;
+	sup_flags |= UBIFS_FLG_PARENTPOINTER;
 
 	sup->ch.node_type  = UBIFS_SB_NODE;
 	sup->key_hash      = UBIFS_KEY_HASH_R5;
@@ -639,6 +640,7 @@ int ubifs_read_superblock(struct ubifs_info *c)
 	c->space_fixup = !!(sup_flags & UBIFS_FLG_SPACE_FIXUP);
 	c->double_hash = !!(sup_flags & UBIFS_FLG_DOUBLE_HASH);
 	c->encrypted = !!(sup_flags & UBIFS_FLG_ENCRYPTION);
+	c->parent_pointer = !!(sup_flags & UBIFS_FLG_PARENTPOINTER);
 
 	if ((sup_flags & ~UBIFS_FLG_MASK) != 0) {
 		ubifs_err(c, "Unknown feature flags found: %#x",
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index cf4cc99b75b5..7560071534bf 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -154,6 +154,7 @@ struct inode *ubifs_iget(struct super_block *sb, unsigned long inum)
 	ui->synced_i_size = ui->ui_size = inode->i_size;
 
 	ui->xattr = (ui->flags & UBIFS_XATTR_FL) ? 1 : 0;
+	ui->parent_inum = le32_to_cpu(ino->parent_inum);
 
 	err = validate_inode(c, inode);
 	if (err)
diff --git a/fs/ubifs/ubifs-media.h b/fs/ubifs/ubifs-media.h
index 5939776c7359..b3cb76cedf20 100644
--- a/fs/ubifs/ubifs-media.h
+++ b/fs/ubifs/ubifs-media.h
@@ -427,15 +427,21 @@ enum {
  * UBIFS_FLG_DOUBLE_HASH: store a 32bit cookie in directory entry nodes to
  *			  support 64bit cookies for lookups by hash
  * UBIFS_FLG_ENCRYPTION: this filesystem contains encrypted files
+ * UBIFS_FLG_PARENTPOINTER: inode nodes maintain a pointer to the parent dir
  */
 enum {
 	UBIFS_FLG_BIGLPT = 0x02,
 	UBIFS_FLG_SPACE_FIXUP = 0x04,
 	UBIFS_FLG_DOUBLE_HASH = 0x08,
 	UBIFS_FLG_ENCRYPTION = 0x10,
+	UBIFS_FLG_PARENTPOINTER = 0x20,
 };
 
-#define UBIFS_FLG_MASK (UBIFS_FLG_BIGLPT|UBIFS_FLG_SPACE_FIXUP|UBIFS_FLG_DOUBLE_HASH|UBIFS_FLG_ENCRYPTION)
+#define UBIFS_FLG_MASK (UBIFS_FLG_BIGLPT		\
+			|UBIFS_FLG_SPACE_FIXUP		\
+			|UBIFS_FLG_DOUBLE_HASH		\
+			|UBIFS_FLG_ENCRYPTION		\
+			|UBIFS_FLG_PARENTPOINTER)
 
 /**
  * struct ubifs_ch - common header node.
@@ -494,7 +500,7 @@ union ubifs_dev_desc {
  * @data_len: inode data length
  * @xattr_cnt: count of extended attributes this inode has
  * @xattr_size: summarized size of all extended attributes in bytes
- * @padding1: reserved for future, zeroes
+ * @parent_inum: parent inode number
  * @xattr_names: sum of lengths of all extended attribute names belonging to
  *               this inode
  * @compr_type: compression type used for this inode
@@ -528,7 +534,7 @@ struct ubifs_ino_node {
 	__le32 data_len;
 	__le32 xattr_cnt;
 	__le32 xattr_size;
-	__u8 padding1[4]; /* Watch 'zero_ino_node_unused()' if changing! */
+	__le32 parent_inum;
 	__le32 xattr_names;
 	__le16 compr_type;
 	__u8 padding2[26]; /* Watch 'zero_ino_node_unused()' if changing! */
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index f14dcc890e47..3c64481f4032 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -353,6 +353,7 @@ struct ubifs_gced_idx_leb {
  *                 currently stored on the flash; used only for regular file
  *                 inodes
  * @ui_size: inode size used by UBIFS when writing to flash
+ * @parent_inum: inode number of the parent directory
  * @flags: inode flags (@UBIFS_COMPR_FL, etc)
  * @compr_type: default compression type used for this inode
  * @last_page_read: page number of last page read (for bulk read)
@@ -404,6 +405,7 @@ struct ubifs_inode {
 	spinlock_t ui_lock;
 	loff_t synced_i_size;
 	loff_t ui_size;
+	ino_t parent_inum;
 	int flags;
 	pgoff_t last_page_read;
 	pgoff_t read_in_a_row;
@@ -1012,6 +1014,7 @@ struct ubifs_debug_info;
  * @space_fixup: flag indicating that free space in LEBs needs to be cleaned up
  * @double_hash: flag indicating that we can do lookups by hash
  * @encrypted: flag indicating that this file system contains encrypted files
+ * @parent_pointer: flag indicating that inodes have pointers to the parent dir
  * @no_chk_data_crc: do not check CRCs when reading data nodes (except during
  *                   recovery)
  * @bulk_read: enable bulk-reads
@@ -1255,6 +1258,7 @@ struct ubifs_info {
 	unsigned int space_fixup:1;
 	unsigned int double_hash:1;
 	unsigned int encrypted:1;
+	unsigned int parent_pointer:1;
 	unsigned int no_chk_data_crc:1;
 	unsigned int bulk_read:1;
 	unsigned int default_compr:2;
-- 
2.12.0

Re: [PATCH 4/6] ubifs: Maintain a parent pointer

[Hyunchul Lee]

Hi Richard,

On Sun, May 21, 2017 at 10:20:49PM +0200, Richard Weinberger wrote:
> The new feature UBIFS_FLG_PARENTPOINTER allows looking
> up the parent. Usually the Linux VFS walks down the filesystem
> and no parent pointers are needed. But when a filesystem
> is exportable via NFS such a lookup is needed.
> We can use a padding field in struct ubifs_ino_node to
> maintain a pointer to the parent inode.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  fs/ubifs/dir.c         | 21 +++++++++++++++++++--
>  fs/ubifs/journal.c     |  5 ++++-
>  fs/ubifs/sb.c          |  2 ++
>  fs/ubifs/super.c       |  1 +
>  fs/ubifs/ubifs-media.h | 12 +++++++++---
>  fs/ubifs/ubifs.h       |  4 ++++
>  6 files changed, 39 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> index e79b529df9c3..a6eadb52a1a8 100644
> --- a/fs/ubifs/dir.c
> +++ b/fs/ubifs/dir.c
> @@ -171,6 +171,7 @@ struct inode *ubifs_new_inode(struct ubifs_info *c, struct inode *dir,
>  	}
>  
>  	inode->i_ino = ++c->highest_inum;
> +	ui->parent_inum = dir->i_ino;
>  	/*
>  	 * The creation sequence number remains with this inode for its
>  	 * lifetime. All nodes for this inode have a greater sequence number,
> @@ -1374,7 +1375,7 @@ static int do_rename(struct inode *old_dir, struct dentry *old_dentry,
>  	if (unlink)
>  		ubifs_assert(inode_is_locked(new_inode));
>  
> -	if (old_dir != new_dir) {
> +	if (move) {
>  		if (ubifs_crypt_is_encrypted(new_dir) &&
>  		    !fscrypt_has_permitted_context(new_dir, old_inode))
>  			return -EPERM;
> @@ -1528,8 +1529,12 @@ static int do_rename(struct inode *old_dir, struct dentry *old_dentry,
>  		mark_inode_dirty(whiteout);
>  		whiteout->i_state &= ~I_LINKABLE;
>  		iput(whiteout);
> +		whiteout_ui->parent_inum = new_dir->i_ino;
>  	}
>  
> +	if (move)
> +		old_inode_ui->parent_inum = new_dir->i_ino;
> +
>  	err = ubifs_jnl_rename(c, old_dir, old_inode, &old_nm, new_dir,
>  			       new_inode, &new_nm, whiteout, sync);

I think that old_inode_ui->parent_inum could point old_dir, even though old_inode
is a child of new_dir. this could happen that there is power-cut before
old_inode is synced. so I guess that old_inode is needed to be written with
rename's node group in ubifs_jnl_rename. is it right?

>  	if (err)
> @@ -1571,6 +1576,8 @@ static int do_rename(struct inode *old_dir, struct dentry *old_dentry,
>  				inc_nlink(old_dir);
>  		}
>  	}
> +	if (move)
> +		old_inode_ui->parent_inum = old_dir->i_ino;
>  	if (whiteout) {
>  		drop_nlink(whiteout);
>  		iput(whiteout);
> @@ -1592,6 +1599,8 @@ static int ubifs_xrename(struct inode *old_dir, struct dentry *old_dentry,
>  	int sync = IS_DIRSYNC(old_dir) || IS_DIRSYNC(new_dir);
>  	struct inode *fst_inode = d_inode(old_dentry);
>  	struct inode *snd_inode = d_inode(new_dentry);
> +	struct ubifs_inode *fst_inode_ui = ubifs_inode(fst_inode);
> +	struct ubifs_inode *snd_inode_ui = ubifs_inode(snd_inode);
>  	struct timespec time;
>  	int err;
>  	struct fscrypt_name fst_nm, snd_nm;
> @@ -1623,7 +1632,10 @@ static int ubifs_xrename(struct inode *old_dir, struct dentry *old_dentry,
>  	old_dir->i_mtime = old_dir->i_ctime = time;
>  	new_dir->i_mtime = new_dir->i_ctime = time;
>  
> -	if (old_dir != new_dir) {
> +	if (new_dir != old_dir) {
> +		fst_inode_ui->parent_inum = new_dir->i_ino;
> +		snd_inode_ui->parent_inum = old_dir->i_ino;
> +
>  		if (S_ISDIR(fst_inode->i_mode) && !S_ISDIR(snd_inode->i_mode)) {
>  			inc_nlink(new_dir);
>  			drop_nlink(old_dir);
> @@ -1637,6 +1649,11 @@ static int ubifs_xrename(struct inode *old_dir, struct dentry *old_dentry,
>  	err = ubifs_jnl_xrename(c, old_dir, fst_inode, &fst_nm, new_dir,
>  				snd_inode, &snd_nm, sync);
>  
> +	if (err && new_dir != old_dir) {
> +		fst_inode_ui->parent_inum = old_dir->i_ino;
> +		snd_inode_ui->parent_inum = new_dir->i_ino;
> +	}
> +
>  	unlock_4_inodes(old_dir, new_dir, NULL, NULL);
>  	ubifs_release_budget(c, &req);
>  
> diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
> index 294519b98874..8eaf8f2f1fe1 100644
> --- a/fs/ubifs/journal.c
> +++ b/fs/ubifs/journal.c
> @@ -66,7 +66,6 @@
>   */
>  static inline void zero_ino_node_unused(struct ubifs_ino_node *ino)
>  {
> -	memset(ino->padding1, 0, 4);
>  	memset(ino->padding2, 0, 26);
>  }
>  
> @@ -470,6 +469,10 @@ static void pack_inode(struct ubifs_info *c, struct ubifs_ino_node *ino,
>  	ino->xattr_cnt   = cpu_to_le32(ui->xattr_cnt);
>  	ino->xattr_size  = cpu_to_le32(ui->xattr_size);
>  	ino->xattr_names = cpu_to_le32(ui->xattr_names);
> +	if (c->parent_pointer)
> +		ino->parent_inum = cpu_to_le32(ui->parent_inum);
> +	else
> +		ino->parent_inum = 0;
>  	zero_ino_node_unused(ino);
>  
>  	/*
> diff --git a/fs/ubifs/sb.c b/fs/ubifs/sb.c
> index 8c25081a5109..f2097c47f629 100644
> --- a/fs/ubifs/sb.c
> +++ b/fs/ubifs/sb.c
> @@ -166,6 +166,7 @@ static int create_default_filesystem(struct ubifs_info *c)
>  	if (big_lpt)
>  		sup_flags |= UBIFS_FLG_BIGLPT;
>  	sup_flags |= UBIFS_FLG_DOUBLE_HASH;
> +	sup_flags |= UBIFS_FLG_PARENTPOINTER;
>  
>  	sup->ch.node_type  = UBIFS_SB_NODE;
>  	sup->key_hash      = UBIFS_KEY_HASH_R5;
> @@ -639,6 +640,7 @@ int ubifs_read_superblock(struct ubifs_info *c)
>  	c->space_fixup = !!(sup_flags & UBIFS_FLG_SPACE_FIXUP);
>  	c->double_hash = !!(sup_flags & UBIFS_FLG_DOUBLE_HASH);
>  	c->encrypted = !!(sup_flags & UBIFS_FLG_ENCRYPTION);
> +	c->parent_pointer = !!(sup_flags & UBIFS_FLG_PARENTPOINTER);
>  
>  	if ((sup_flags & ~UBIFS_FLG_MASK) != 0) {
>  		ubifs_err(c, "Unknown feature flags found: %#x",
> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index cf4cc99b75b5..7560071534bf 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
> @@ -154,6 +154,7 @@ struct inode *ubifs_iget(struct super_block *sb, unsigned long inum)
>  	ui->synced_i_size = ui->ui_size = inode->i_size;
>  
>  	ui->xattr = (ui->flags & UBIFS_XATTR_FL) ? 1 : 0;
> +	ui->parent_inum = le32_to_cpu(ino->parent_inum);
>  
>  	err = validate_inode(c, inode);
>  	if (err)
> diff --git a/fs/ubifs/ubifs-media.h b/fs/ubifs/ubifs-media.h
> index 5939776c7359..b3cb76cedf20 100644
> --- a/fs/ubifs/ubifs-media.h
> +++ b/fs/ubifs/ubifs-media.h
> @@ -427,15 +427,21 @@ enum {
>   * UBIFS_FLG_DOUBLE_HASH: store a 32bit cookie in directory entry nodes to
>   *			  support 64bit cookies for lookups by hash
>   * UBIFS_FLG_ENCRYPTION: this filesystem contains encrypted files
> + * UBIFS_FLG_PARENTPOINTER: inode nodes maintain a pointer to the parent dir
>   */
>  enum {
>  	UBIFS_FLG_BIGLPT = 0x02,
>  	UBIFS_FLG_SPACE_FIXUP = 0x04,
>  	UBIFS_FLG_DOUBLE_HASH = 0x08,
>  	UBIFS_FLG_ENCRYPTION = 0x10,
> +	UBIFS_FLG_PARENTPOINTER = 0x20,
>  };
>  
> -#define UBIFS_FLG_MASK (UBIFS_FLG_BIGLPT|UBIFS_FLG_SPACE_FIXUP|UBIFS_FLG_DOUBLE_HASH|UBIFS_FLG_ENCRYPTION)
> +#define UBIFS_FLG_MASK (UBIFS_FLG_BIGLPT		\
> +			|UBIFS_FLG_SPACE_FIXUP		\
> +			|UBIFS_FLG_DOUBLE_HASH		\
> +			|UBIFS_FLG_ENCRYPTION		\
> +			|UBIFS_FLG_PARENTPOINTER)
>  
>  /**
>   * struct ubifs_ch - common header node.
> @@ -494,7 +500,7 @@ union ubifs_dev_desc {
>   * @data_len: inode data length
>   * @xattr_cnt: count of extended attributes this inode has
>   * @xattr_size: summarized size of all extended attributes in bytes
> - * @padding1: reserved for future, zeroes
> + * @parent_inum: parent inode number
>   * @xattr_names: sum of lengths of all extended attribute names belonging to
>   *               this inode
>   * @compr_type: compression type used for this inode
> @@ -528,7 +534,7 @@ struct ubifs_ino_node {
>  	__le32 data_len;
>  	__le32 xattr_cnt;
>  	__le32 xattr_size;
> -	__u8 padding1[4]; /* Watch 'zero_ino_node_unused()' if changing! */
> +	__le32 parent_inum;
>  	__le32 xattr_names;
>  	__le16 compr_type;
>  	__u8 padding2[26]; /* Watch 'zero_ino_node_unused()' if changing! */
> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
> index f14dcc890e47..3c64481f4032 100644
> --- a/fs/ubifs/ubifs.h
> +++ b/fs/ubifs/ubifs.h
> @@ -353,6 +353,7 @@ struct ubifs_gced_idx_leb {
>   *                 currently stored on the flash; used only for regular file
>   *                 inodes
>   * @ui_size: inode size used by UBIFS when writing to flash
> + * @parent_inum: inode number of the parent directory
>   * @flags: inode flags (@UBIFS_COMPR_FL, etc)
>   * @compr_type: default compression type used for this inode
>   * @last_page_read: page number of last page read (for bulk read)
> @@ -404,6 +405,7 @@ struct ubifs_inode {
>  	spinlock_t ui_lock;
>  	loff_t synced_i_size;
>  	loff_t ui_size;
> +	ino_t parent_inum;
>  	int flags;
>  	pgoff_t last_page_read;
>  	pgoff_t read_in_a_row;
> @@ -1012,6 +1014,7 @@ struct ubifs_debug_info;
>   * @space_fixup: flag indicating that free space in LEBs needs to be cleaned up
>   * @double_hash: flag indicating that we can do lookups by hash
>   * @encrypted: flag indicating that this file system contains encrypted files
> + * @parent_pointer: flag indicating that inodes have pointers to the parent dir
>   * @no_chk_data_crc: do not check CRCs when reading data nodes (except during
>   *                   recovery)
>   * @bulk_read: enable bulk-reads
> @@ -1255,6 +1258,7 @@ struct ubifs_info {
>  	unsigned int space_fixup:1;
>  	unsigned int double_hash:1;
>  	unsigned int encrypted:1;
> +	unsigned int parent_pointer:1;
>  	unsigned int no_chk_data_crc:1;
>  	unsigned int bulk_read:1;
>  	unsigned int default_compr:2;
> -- 
> 2.12.0
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

-- 

Thanks,
Hyunchul

Re: [PATCH 4/6] ubifs: Maintain a parent pointer

[Richard Weinberger]

Hyunchul,

Am 22.05.2017 um 06:30 schrieb Hyunchul Lee:
>> +	if (move)
>> +		old_inode_ui->parent_inum = new_dir->i_ino;
>> +
>>  	err = ubifs_jnl_rename(c, old_dir, old_inode, &old_nm, new_dir,
>>  			       new_inode, &new_nm, whiteout, sync);
> 
> I think that old_inode_ui->parent_inum could point old_dir, even though old_inode
> is a child of new_dir. this could happen that there is power-cut before
> old_inode is synced. so I guess that old_inode is needed to be written with
> rename's node group in ubifs_jnl_rename. is it right?

I assumed that the journal does this already because we change old_inode->i_ctime
in this function too.
But checking the code showed the opposite.
So, if we face a power-cut the rename can succeed but we lose the ctime change.

This needs to be addressed before we can add the parent pointer.

Thanks,
//richard

Re: [PATCH 4/6] ubifs: Maintain a parent pointer

[Hyunchul Lee]

Hi Richard,

On Mon, May 22, 2017 at 10:45:08AM +0200, Richard Weinberger wrote:
> Hyunchul,
> 
> Am 22.05.2017 um 06:30 schrieb Hyunchul Lee:
> >> +	if (move)
> >> +		old_inode_ui->parent_inum = new_dir->i_ino;
> >> +
> >>  	err = ubifs_jnl_rename(c, old_dir, old_inode, &old_nm, new_dir,
> >>  			       new_inode, &new_nm, whiteout, sync);
> > 
> > I think that old_inode_ui->parent_inum could point old_dir, even though old_inode
> > is a child of new_dir. this could happen that there is power-cut before
> > old_inode is synced. so I guess that old_inode is needed to be written with
> > rename's node group in ubifs_jnl_rename. is it right?
> 
> I assumed that the journal does this already because we change old_inode->i_ctime
> in this function too.
> But checking the code showed the opposite.
> So, if we face a power-cut the rename can succeed but we lose the ctime change.
> 
> This needs to be addressed before we can add the parent pointer.

Is writing old_inode->i_ctime required? I guess that it is needed only when 
IS_SYNC(old_inode) is true, otherwise we don't need to guarantee that ctime
is synced.

> 
> Thanks,
> //richard
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

-- 

Thanks,
Hyunchul

Re: [PATCH 4/6] ubifs: Maintain a parent pointer

[Richard Weinberger]

Hyunchul,

Am 23.05.2017 um 01:50 schrieb Hyunchul Lee:
>> I assumed that the journal does this already because we change old_inode->i_ctime
>> in this function too.
>> But checking the code showed the opposite.
>> So, if we face a power-cut the rename can succeed but we lose the ctime change.
>>
>> This needs to be addressed before we can add the parent pointer.
> 
> Is writing old_inode->i_ctime required? I guess that it is needed only when 
> IS_SYNC(old_inode) is true, otherwise we don't need to guarantee that ctime
> is synced.

Well, rename() has to be an atomic operation. When UBIFS updates ctime during this
operation it has also to be atomic. Otherwise, after a power-cut, you can end-up with
a renamed file but a not updated ctime.
Userspace can depend on this. Think of a backup program that needs to detect changes.
It won't notice the renamed file.

AFAICT POSIX does not require ctime to be updated during rename() but suggests it.
My point is, either we do it always or never.

Just checked xfs and btrfs. Both seem to update ctime of old_inode during the rename
transaction. UBIFS should do the same.

Thanks,
//richard

Re: [PATCH 4/6] ubifs: Maintain a parent pointer

[Christoph Hellwig]

On Sun, May 21, 2017 at 10:20:49PM +0200, Richard Weinberger wrote:
> The new feature UBIFS_FLG_PARENTPOINTER allows looking
> up the parent. Usually the Linux VFS walks down the filesystem
> and no parent pointers are needed. But when a filesystem
> is exportable via NFS such a lookup is needed.
> We can use a padding field in struct ubifs_ino_node to
> maintain a pointer to the parent inode.

Note that nfsd/exportfs will only do this for directories, and beacause
we hardlinks for other objects that is an important distinction.

I think the ubifs code should make sure to only maintain the parent
pointers for directories, otherwise confusion and abuses will happen.

Re: [PATCH 4/6] ubifs: Maintain a parent pointer

[Richard Weinberger]

Christoph,

Am 23.05.2017 um 10:37 schrieb Christoph Hellwig:
> On Sun, May 21, 2017 at 10:20:49PM +0200, Richard Weinberger wrote:
>> The new feature UBIFS_FLG_PARENTPOINTER allows looking
>> up the parent. Usually the Linux VFS walks down the filesystem
>> and no parent pointers are needed. But when a filesystem
>> is exportable via NFS such a lookup is needed.
>> We can use a padding field in struct ubifs_ino_node to
>> maintain a pointer to the parent inode.
> 
> Note that nfsd/exportfs will only do this for directories, and beacause
> we hardlinks for other objects that is an important distinction.
> 
> I think the ubifs code should make sure to only maintain the parent
> pointers for directories, otherwise confusion and abuses will happen.

Thanks for the info, I'll massage the code in that direction.

Thanks,
//richard