All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] ext4: return the extent cache information via fiemap
@ 2019-08-09 18:18 Theodore Ts'o
  2019-08-09 18:18 ` [PATCH 2/3] ext4: add a new ioctl EXT4_IOC_CLEAR_ES_CACHE Theodore Ts'o
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Theodore Ts'o @ 2019-08-09 18:18 UTC (permalink / raw)
  To: Ext4 Developers List; +Cc: Theodore Ts'o

For debugging reasons, it's useful to know the contents of the extent
cache.  Since the extent cache contains much of what is in the fiemap
ioctl, extend the fiemap interface to return this information via some
ext4-specific flags.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 fs/ext4/ext4.h           | 11 +++++++++
 fs/ext4/extents.c        | 50 ++++++++++++++++++++++++++++++++++++++++
 fs/ext4/extents_status.c | 20 ++++++++++++++--
 fs/ext4/extents_status.h |  3 +++
 4 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index bf660aa7a9e0..36954d951dff 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -679,6 +679,17 @@ enum {
 #define EXT4_IOC32_SETVERSION_OLD	FS_IOC32_SETVERSION
 #endif
 
+/* Extra ext4-specific fiemap flags */
+#define EXT4_FIEMAP_FLAG_EXTENT_CACHE	0x08000000 /* return the extent status cache */
+
+/* These flags are ext4 specific fiemap flags */
+#define EXT4_SPECIFIC_FIEMAP_FLAGS	(FIEMAP_FLAG_CACHE |\
+					 EXT4_FIEMAP_FLAG_EXTENT_CACHE)
+
+#define EXT4_FIEMAP_EXTENT_HOLE		0x08000000 /* Entry in extent status
+						      cache for a hole*/
+
+
 /* Max physical block we can address w/o extents */
 #define EXT4_MAX_BLOCK_FILE_PHYS	0xFFFFFFFF
 
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 92266a2da7d6..7e4aee57c2f3 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -2179,6 +2179,46 @@ static int ext4_fill_fiemap_extents(struct inode *inode,
 	unsigned int flags = 0;
 	unsigned char blksize_bits = inode->i_sb->s_blocksize_bits;
 
+	if (fieinfo->fi_flags & EXT4_FIEMAP_FLAG_EXTENT_CACHE) {
+		ext4_lblk_t next, end = block + num - 1;
+		struct extent_status es;
+		unsigned int flags;
+		int err;
+
+		while (block <= end) {
+			next = 0;
+			flags = 0;
+			if (!ext4_es_lookup_extent2(inode, block, &next, &es))
+				break;
+			if (ext4_es_is_unwritten(&es))
+				flags |= FIEMAP_EXTENT_UNWRITTEN;
+			if (ext4_es_is_delayed(&es))
+				flags |= (FIEMAP_EXTENT_DELALLOC |
+					  FIEMAP_EXTENT_UNKNOWN);
+			if (ext4_es_is_hole(&es))
+				flags |= EXT4_FIEMAP_EXTENT_HOLE;
+			if (next == 0)
+				flags |= FIEMAP_EXTENT_LAST;
+			if (flags & (FIEMAP_EXTENT_DELALLOC|
+				     EXT4_FIEMAP_EXTENT_HOLE))
+				es.es_pblk = 0;
+			else
+				es.es_pblk = ext4_es_pblock(&es);
+			err = fiemap_fill_next_extent(fieinfo,
+				(__u64)es.es_lblk << blksize_bits,
+				(__u64)es.es_pblk << blksize_bits,
+				(__u64)es.es_len << blksize_bits,
+				flags);
+			if (next == 0)
+				break;
+			block = next;
+			if (err < 0)
+				return err;
+			if (err == 1)
+				return 0;
+		}
+		return 0;
+	}
 	while (block < last && block != EXT_MAX_BLOCKS) {
 		num = last - block;
 		/* find extent for this block */
@@ -5059,6 +5099,7 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 		__u64 start, __u64 len)
 {
 	ext4_lblk_t start_blk;
+	unsigned int flags = fieinfo->fi_flags;
 	int error = 0;
 
 	if (ext4_has_inline_data(inode)) {
@@ -5077,6 +5118,13 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 			return error;
 	}
 
+	/*
+	 * Mask out the ext4-specific flags since otherwise
+	 * generic_block_fiemap and fiemap_check_flags will get
+	 * cranky.
+	 */
+	fieinfo->fi_flags &= ~EXT4_SPECIFIC_FIEMAP_FLAGS;
+
 	/* fallback to generic here if not in extents fmt */
 	if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)))
 		return generic_block_fiemap(inode, fieinfo, start, len,
@@ -5085,6 +5133,8 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 	if (fiemap_check_flags(fieinfo, EXT4_FIEMAP_FLAGS))
 		return -EBADR;
 
+	fieinfo->fi_flags = flags;
+
 	if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) {
 		error = ext4_xattr_fiemap(inode, fieinfo);
 	} else {
diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index 7521de2dcf3a..6611f06c6cdc 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -898,8 +898,9 @@ void ext4_es_cache_extent(struct inode *inode, ext4_lblk_t lblk,
  *
  * Return: 1 on found, 0 on not
  */
-int ext4_es_lookup_extent(struct inode *inode, ext4_lblk_t lblk,
-			  struct extent_status *es)
+int ext4_es_lookup_extent2(struct inode *inode, ext4_lblk_t lblk,
+			   ext4_lblk_t *next_lblk,
+			   struct extent_status *es)
 {
 	struct ext4_es_tree *tree;
 	struct ext4_es_stats *stats;
@@ -948,6 +949,15 @@ int ext4_es_lookup_extent(struct inode *inode, ext4_lblk_t lblk,
 		if (!ext4_es_is_referenced(es1))
 			ext4_es_set_referenced(es1);
 		stats->es_stats_cache_hits++;
+		if (next_lblk) {
+			node = rb_next(&es1->rb_node);
+			if (node) {
+				es1 = rb_entry(node, struct extent_status,
+					       rb_node);
+				*next_lblk = es1->es_lblk;
+			} else
+				*next_lblk = 0;
+		}
 	} else {
 		stats->es_stats_cache_misses++;
 	}
@@ -958,6 +968,12 @@ int ext4_es_lookup_extent(struct inode *inode, ext4_lblk_t lblk,
 	return found;
 }
 
+int ext4_es_lookup_extent(struct inode *inode, ext4_lblk_t lblk,
+			  struct extent_status *es)
+{
+	return ext4_es_lookup_extent2(inode, lblk, NULL, es);
+}
+
 static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
 			      ext4_lblk_t end)
 {
diff --git a/fs/ext4/extents_status.h b/fs/ext4/extents_status.h
index 131a8b7df265..391b5251a891 100644
--- a/fs/ext4/extents_status.h
+++ b/fs/ext4/extents_status.h
@@ -141,6 +141,9 @@ extern void ext4_es_find_extent_range(struct inode *inode,
 				      struct extent_status *es);
 extern int ext4_es_lookup_extent(struct inode *inode, ext4_lblk_t lblk,
 				 struct extent_status *es);
+extern int ext4_es_lookup_extent2(struct inode *inode, ext4_lblk_t lblk,
+				  ext4_lblk_t *next_lblk,
+				  struct extent_status *es);
 extern bool ext4_es_scan_range(struct inode *inode,
 			       int (*matching_fn)(struct extent_status *es),
 			       ext4_lblk_t lblk, ext4_lblk_t end);
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/3] ext4: add a new ioctl EXT4_IOC_CLEAR_ES_CACHE
  2019-08-09 18:18 [PATCH 1/3] ext4: return the extent cache information via fiemap Theodore Ts'o
@ 2019-08-09 18:18 ` Theodore Ts'o
  2019-08-09 18:18 ` [PATCH 3/3] ext4: add a new ioctl EXT4_IOC_GETSTATE Theodore Ts'o
  2019-08-10  7:33 ` [PATCH 1/3] ext4: return the extent cache information via fiemap Christoph Hellwig
  2 siblings, 0 replies; 9+ messages in thread
From: Theodore Ts'o @ 2019-08-09 18:18 UTC (permalink / raw)
  To: Ext4 Developers List; +Cc: Theodore Ts'o

The new ioctl EXT4_IOC_CLEAR_ES_CACHE will force an inode's extent
status cache to be cleared out.  This is intended for use for
debugging.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 fs/ext4/ext4.h           |  2 ++
 fs/ext4/extents_status.c | 29 +++++++++++++++++++++++++++++
 fs/ext4/extents_status.h |  1 +
 fs/ext4/ioctl.c          |  9 +++++++++
 4 files changed, 41 insertions(+)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 36954d951dff..f6c305b43ffa 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -649,6 +649,8 @@ enum {
 #define EXT4_IOC_SET_ENCRYPTION_POLICY	FS_IOC_SET_ENCRYPTION_POLICY
 #define EXT4_IOC_GET_ENCRYPTION_PWSALT	FS_IOC_GET_ENCRYPTION_PWSALT
 #define EXT4_IOC_GET_ENCRYPTION_POLICY	FS_IOC_GET_ENCRYPTION_POLICY
+/* ioctl codes 19--2F are reserved for fscrypt */
+#define EXT4_IOC_CLEAR_ES_CACHE		_IO('f', 30)
 
 #define EXT4_IOC_FSGETXATTR		FS_IOC_FSGETXATTR
 #define EXT4_IOC_FSSETXATTR		FS_IOC_FSSETXATTR
diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index 6611f06c6cdc..95cfd2370bb0 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -1390,6 +1390,35 @@ static int es_reclaim_extents(struct ext4_inode_info *ei, int *nr_to_scan)
 	return nr_shrunk;
 }
 
+/*
+ * Called to support EXT4_IOC_CLEAR_ES_CACHE.  We can only remove
+ * discretionary entries from the extent status cache.  (Some entries
+ * must be present for proper operations.)
+ */
+void ext4_clear_inode_es(struct inode *inode)
+{
+	struct ext4_inode_info *ei = EXT4_I(inode);
+	struct ext4_es_tree *tree;
+	struct rb_node *node;
+
+	write_lock(&ei->i_es_lock);
+	tree = &EXT4_I(inode)->i_es_tree;
+	tree->cache_es = NULL;
+	node = rb_first(&tree->root);
+	while (node) {
+		struct extent_status *es;
+		es = rb_entry(node, struct extent_status, rb_node);
+
+		node = rb_next(node);
+		if (!ext4_es_is_delayed(es)) {
+			rb_erase(&es->rb_node, &tree->root);
+			ext4_es_free_extent(inode, es);
+		}
+	}
+	ext4_clear_inode_state(inode, EXT4_STATE_EXT_PRECACHED);
+	write_unlock(&ei->i_es_lock);
+}
+
 #ifdef ES_DEBUG__
 static void ext4_print_pending_tree(struct inode *inode)
 {
diff --git a/fs/ext4/extents_status.h b/fs/ext4/extents_status.h
index 391b5251a891..ee7c1530f1f1 100644
--- a/fs/ext4/extents_status.h
+++ b/fs/ext4/extents_status.h
@@ -251,5 +251,6 @@ extern unsigned int ext4_es_delayed_clu(struct inode *inode, ext4_lblk_t lblk,
 					ext4_lblk_t len);
 extern void ext4_es_remove_blks(struct inode *inode, ext4_lblk_t lblk,
 				ext4_lblk_t len);
+extern void ext4_clear_inode_es(struct inode *inode);
 
 #endif /* _EXT4_EXTENTS_STATUS_H */
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 442f7ef873fc..15b1047878ab 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -1115,6 +1115,14 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	case EXT4_IOC_GET_ENCRYPTION_POLICY:
 		return fscrypt_ioctl_get_policy(filp, (void __user *)arg);
 
+	case EXT4_IOC_CLEAR_ES_CACHE:
+	{
+		if (!inode_owner_or_capable(inode))
+			return -EACCES;
+		ext4_clear_inode_es(inode);
+		return 0;
+	}
+
 	case EXT4_IOC_FSGETXATTR:
 	{
 		struct fsxattr fa;
@@ -1233,6 +1241,7 @@ long ext4_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case EXT4_IOC_GET_ENCRYPTION_POLICY:
 	case EXT4_IOC_SHUTDOWN:
 	case FS_IOC_GETFSMAP:
+	case EXT4_IOC_CLEAR_ES_CACHE:
 		break;
 	default:
 		return -ENOIOCTLCMD;
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/3] ext4: add a new ioctl EXT4_IOC_GETSTATE
  2019-08-09 18:18 [PATCH 1/3] ext4: return the extent cache information via fiemap Theodore Ts'o
  2019-08-09 18:18 ` [PATCH 2/3] ext4: add a new ioctl EXT4_IOC_CLEAR_ES_CACHE Theodore Ts'o
@ 2019-08-09 18:18 ` Theodore Ts'o
  2019-08-09 19:18   ` Eric Biggers
  2019-08-10  7:33 ` [PATCH 1/3] ext4: return the extent cache information via fiemap Christoph Hellwig
  2 siblings, 1 reply; 9+ messages in thread
From: Theodore Ts'o @ 2019-08-09 18:18 UTC (permalink / raw)
  To: Ext4 Developers List; +Cc: Theodore Ts'o

The new ioctl EXT4_IOC_GETSTATE returns some of the dynamic state of
an ext4 inode for debugging purposes.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 fs/ext4/ext4.h  | 11 +++++++++++
 fs/ext4/ioctl.c | 17 +++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index f6c305b43ffa..58b7a0905186 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -651,6 +651,7 @@ enum {
 #define EXT4_IOC_GET_ENCRYPTION_POLICY	FS_IOC_GET_ENCRYPTION_POLICY
 /* ioctl codes 19--2F are reserved for fscrypt */
 #define EXT4_IOC_CLEAR_ES_CACHE		_IO('f', 30)
+#define EXT4_IOC_GETSTATE		_IOW('f', 30, __u32)
 
 #define EXT4_IOC_FSGETXATTR		FS_IOC_FSGETXATTR
 #define EXT4_IOC_FSSETXATTR		FS_IOC_FSSETXATTR
@@ -664,6 +665,16 @@ enum {
 #define EXT4_GOING_FLAGS_LOGFLUSH		0x1	/* flush log but not data */
 #define EXT4_GOING_FLAGS_NOLOGFLUSH		0x2	/* don't flush log nor data */
 
+/*
+ * Flags returned by EXT4_IOC_GETSTATE
+ *
+ * We only expose to userspace a subset of the state flags in
+ * i_state_flags
+ */
+#define EXT4_STATE_FLAG_EXT_PRECACHED	0x00000001
+#define EXT4_STATE_FLAG_NEW		0x00000002
+#define EXT4_STATE_FLAG_NEWENTRY	0x00000004
+#define EXT4_STATE_FLAG_DA_ALLOC_CLOSE	0x00000008
 
 #if defined(__KERNEL__) && defined(CONFIG_COMPAT)
 /*
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 15b1047878ab..ffb7bde4900d 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -1123,6 +1123,22 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		return 0;
 	}
 
+	case EXT4_IOC_GETSTATE:
+	{
+		__u32	state = 0;
+
+		if (ext4_test_inode_state(inode, EXT4_STATE_EXT_PRECACHED))
+			state |= EXT4_STATE_FLAG_EXT_PRECACHED;
+		if (ext4_test_inode_state(inode, EXT4_STATE_NEW))
+			state |= EXT4_STATE_FLAG_NEW;
+		if (ext4_test_inode_state(inode, EXT4_STATE_NEWENTRY))
+			state |= EXT4_STATE_FLAG_NEWENTRY;
+		if (ext4_test_inode_state(inode, EXT4_STATE_DA_ALLOC_CLOSE))
+			state |= EXT4_STATE_FLAG_DA_ALLOC_CLOSE;
+
+		return put_user(state, (__u32 __user *) arg);
+	}
+
 	case EXT4_IOC_FSGETXATTR:
 	{
 		struct fsxattr fa;
@@ -1242,6 +1258,7 @@ long ext4_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case EXT4_IOC_SHUTDOWN:
 	case FS_IOC_GETFSMAP:
 	case EXT4_IOC_CLEAR_ES_CACHE:
+	case EXT4_IOC_GETSTATE:
 		break;
 	default:
 		return -ENOIOCTLCMD;
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] ext4: add a new ioctl EXT4_IOC_GETSTATE
  2019-08-09 18:18 ` [PATCH 3/3] ext4: add a new ioctl EXT4_IOC_GETSTATE Theodore Ts'o
@ 2019-08-09 19:18   ` Eric Biggers
  2019-08-10  0:12     ` Theodore Y. Ts'o
  0 siblings, 1 reply; 9+ messages in thread
From: Eric Biggers @ 2019-08-09 19:18 UTC (permalink / raw)
  To: Theodore Ts'o; +Cc: Ext4 Developers List

On Fri, Aug 09, 2019 at 02:18:31PM -0400, Theodore Ts'o wrote:
> The new ioctl EXT4_IOC_GETSTATE returns some of the dynamic state of
> an ext4 inode for debugging purposes.
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  fs/ext4/ext4.h  | 11 +++++++++++
>  fs/ext4/ioctl.c | 17 +++++++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index f6c305b43ffa..58b7a0905186 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -651,6 +651,7 @@ enum {
>  #define EXT4_IOC_GET_ENCRYPTION_POLICY	FS_IOC_GET_ENCRYPTION_POLICY
>  /* ioctl codes 19--2F are reserved for fscrypt */
>  #define EXT4_IOC_CLEAR_ES_CACHE		_IO('f', 30)
> +#define EXT4_IOC_GETSTATE		_IOW('f', 30, __u32)

30 == 0x1e overlaps with the range claimed to be reserved for fscrypt.

Also, these two new ioctls are both number 30, which means they can't be
controlled separately by SELinux, which only looks at the number.

- Eric

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] ext4: add a new ioctl EXT4_IOC_GETSTATE
  2019-08-09 19:18   ` Eric Biggers
@ 2019-08-10  0:12     ` Theodore Y. Ts'o
  2019-08-11 21:38       ` Eric Biggers
  0 siblings, 1 reply; 9+ messages in thread
From: Theodore Y. Ts'o @ 2019-08-10  0:12 UTC (permalink / raw)
  To: Ext4 Developers List

On Fri, Aug 09, 2019 at 12:18:12PM -0700, Eric Biggers wrote:
> On Fri, Aug 09, 2019 at 02:18:31PM -0400, Theodore Ts'o wrote:
> > The new ioctl EXT4_IOC_GETSTATE returns some of the dynamic state of
> > an ext4 inode for debugging purposes.
> > 
> > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > ---
> >  fs/ext4/ext4.h  | 11 +++++++++++
> >  fs/ext4/ioctl.c | 17 +++++++++++++++++
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> > index f6c305b43ffa..58b7a0905186 100644
> > --- a/fs/ext4/ext4.h
> > +++ b/fs/ext4/ext4.h
> > @@ -651,6 +651,7 @@ enum {
> >  #define EXT4_IOC_GET_ENCRYPTION_POLICY	FS_IOC_GET_ENCRYPTION_POLICY
> >  /* ioctl codes 19--2F are reserved for fscrypt */
> >  #define EXT4_IOC_CLEAR_ES_CACHE		_IO('f', 30)
> > +#define EXT4_IOC_GETSTATE		_IOW('f', 30, __u32)
> 
> 30 == 0x1e overlaps with the range claimed to be reserved for fscrypt.
> 
> Also, these two new ioctls are both number 30, which means they can't be
> controlled separately by SELinux, which only looks at the number.

Yeah, that was my screw up.  The range reservation for fscrypt was
intended to be in decimal starting with 19 decimal
(FSIOC_SET_ENCRYPTION_POLICY), and I believe with the new key
management we were up to 26?  So If I reserve up to 39, that should be
more than enough, do you agree?

I'll then make EXT4_IOC_CLEAR_ES_CACHE 40 and EXT4_IOC_GETSTATE 41.

If we're in agreement, then I'll add an update to
Documentation/ioctl/ioctl-number.rst, which is badly out of date with
respect to the ioctl's used in ext2 and ext4 (and of course ext3 has
since been removed from the kernel tree).

						- Ted

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/3] ext4: return the extent cache information via fiemap
  2019-08-09 18:18 [PATCH 1/3] ext4: return the extent cache information via fiemap Theodore Ts'o
  2019-08-09 18:18 ` [PATCH 2/3] ext4: add a new ioctl EXT4_IOC_CLEAR_ES_CACHE Theodore Ts'o
  2019-08-09 18:18 ` [PATCH 3/3] ext4: add a new ioctl EXT4_IOC_GETSTATE Theodore Ts'o
@ 2019-08-10  7:33 ` Christoph Hellwig
  2019-08-11 16:15   ` Theodore Y. Ts'o
  2 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2019-08-10  7:33 UTC (permalink / raw)
  To: Theodore Ts'o; +Cc: Ext4 Developers List, linux-fsdevel, linux-api


On Fri, Aug 09, 2019 at 02:18:29PM -0400, Theodore Ts'o wrote:
> For debugging reasons, it's useful to know the contents of the extent
> cache.  Since the extent cache contains much of what is in the fiemap
> ioctl, extend the fiemap interface to return this information via some
> ext4-specific flags.

Nak.  No weird fs specific fiemap flags that aren't even in the uapi
header.  Please provide your own debug only interface.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/3] ext4: return the extent cache information via fiemap
  2019-08-10  7:33 ` [PATCH 1/3] ext4: return the extent cache information via fiemap Christoph Hellwig
@ 2019-08-11 16:15   ` Theodore Y. Ts'o
  2019-08-12  6:47     ` Christoph Hellwig
  0 siblings, 1 reply; 9+ messages in thread
From: Theodore Y. Ts'o @ 2019-08-11 16:15 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Ext4 Developers List, linux-fsdevel, linux-api

On Sat, Aug 10, 2019 at 12:33:43AM -0700, Christoph Hellwig wrote:
> 
> On Fri, Aug 09, 2019 at 02:18:29PM -0400, Theodore Ts'o wrote:
> > For debugging reasons, it's useful to know the contents of the extent
> > cache.  Since the extent cache contains much of what is in the fiemap
> > ioctl, extend the fiemap interface to return this information via some
> > ext4-specific flags.
> 
> Nak.  No weird fs specific fiemap flags that aren't even in the uapi
> header.  Please provide your own debug only interface.

I can understand why you don't like this from the principle of the
thing.

I'll create my own ioctl, and make a copy of ioctl_fiemap() into ext4
and modify it for my needs.  I was trying to avoid needing to do that,
since there is plenty of space in the fiemap flags to carve out space
for file-specific specific flags, and avoiding making extra copies of
code for the purposes of reuse weighed more heavily than "no
fs-specific fiemap flags".

						- Ted

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] ext4: add a new ioctl EXT4_IOC_GETSTATE
  2019-08-10  0:12     ` Theodore Y. Ts'o
@ 2019-08-11 21:38       ` Eric Biggers
  0 siblings, 0 replies; 9+ messages in thread
From: Eric Biggers @ 2019-08-11 21:38 UTC (permalink / raw)
  To: Theodore Y. Ts'o; +Cc: Ext4 Developers List

On Fri, Aug 09, 2019 at 08:12:47PM -0400, Theodore Y. Ts'o wrote:
> On Fri, Aug 09, 2019 at 12:18:12PM -0700, Eric Biggers wrote:
> > On Fri, Aug 09, 2019 at 02:18:31PM -0400, Theodore Ts'o wrote:
> > > The new ioctl EXT4_IOC_GETSTATE returns some of the dynamic state of
> > > an ext4 inode for debugging purposes.
> > > 
> > > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > > ---
> > >  fs/ext4/ext4.h  | 11 +++++++++++
> > >  fs/ext4/ioctl.c | 17 +++++++++++++++++
> > >  2 files changed, 28 insertions(+)
> > > 
> > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> > > index f6c305b43ffa..58b7a0905186 100644
> > > --- a/fs/ext4/ext4.h
> > > +++ b/fs/ext4/ext4.h
> > > @@ -651,6 +651,7 @@ enum {
> > >  #define EXT4_IOC_GET_ENCRYPTION_POLICY	FS_IOC_GET_ENCRYPTION_POLICY
> > >  /* ioctl codes 19--2F are reserved for fscrypt */
> > >  #define EXT4_IOC_CLEAR_ES_CACHE		_IO('f', 30)
> > > +#define EXT4_IOC_GETSTATE		_IOW('f', 30, __u32)
> > 
> > 30 == 0x1e overlaps with the range claimed to be reserved for fscrypt.
> > 
> > Also, these two new ioctls are both number 30, which means they can't be
> > controlled separately by SELinux, which only looks at the number.
> 
> Yeah, that was my screw up.  The range reservation for fscrypt was
> intended to be in decimal starting with 19 decimal
> (FSIOC_SET_ENCRYPTION_POLICY), and I believe with the new key
> management we were up to 26?  So If I reserve up to 39, that should be
> more than enough, do you agree?
> 
> I'll then make EXT4_IOC_CLEAR_ES_CACHE 40 and EXT4_IOC_GETSTATE 41.
> 
> If we're in agreement, then I'll add an update to
> Documentation/ioctl/ioctl-number.rst, which is badly out of date with
> respect to the ioctl's used in ext2 and ext4 (and of course ext3 has
> since been removed from the kernel tree).
> 

Sounds good to me.

- Eric

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/3] ext4: return the extent cache information via fiemap
  2019-08-11 16:15   ` Theodore Y. Ts'o
@ 2019-08-12  6:47     ` Christoph Hellwig
  0 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2019-08-12  6:47 UTC (permalink / raw)
  To: Theodore Y. Ts'o
  Cc: Christoph Hellwig, Ext4 Developers List, linux-fsdevel, linux-api

On Sun, Aug 11, 2019 at 12:15:08PM -0400, Theodore Y. Ts'o wrote:
> > Nak.  No weird fs specific fiemap flags that aren't even in the uapi
> > header.  Please provide your own debug only interface.
> 
> I can understand why you don't like this from the principle of the
> thing.

I think mixing up in-kernel temporary state with the on-disk layout
is a bad idea, yes.  I think it is an even worse idea to try to sneak
it in without API review in an undocumented fashion.

> 
> I'll create my own ioctl, and make a copy of ioctl_fiemap() into ext4
> and modify it for my needs.  I was trying to avoid needing to do that,
> since there is plenty of space in the fiemap flags to carve out space
> for file-specific specific flags, and avoiding making extra copies of
> code for the purposes of reuse weighed more heavily than "no
> fs-specific fiemap flags".

I think dumping your internal state is a candidate for debugs, not
a copy of ioctl_fiemap.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-08-12  6:47 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-09 18:18 [PATCH 1/3] ext4: return the extent cache information via fiemap Theodore Ts'o
2019-08-09 18:18 ` [PATCH 2/3] ext4: add a new ioctl EXT4_IOC_CLEAR_ES_CACHE Theodore Ts'o
2019-08-09 18:18 ` [PATCH 3/3] ext4: add a new ioctl EXT4_IOC_GETSTATE Theodore Ts'o
2019-08-09 19:18   ` Eric Biggers
2019-08-10  0:12     ` Theodore Y. Ts'o
2019-08-11 21:38       ` Eric Biggers
2019-08-10  7:33 ` [PATCH 1/3] ext4: return the extent cache information via fiemap Christoph Hellwig
2019-08-11 16:15   ` Theodore Y. Ts'o
2019-08-12  6:47     ` Christoph Hellwig

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.