diff mbox series

[v4,01/15] fs: Rename the kernel-internal data lifetime constants

Message ID 20231114214132.1486867-2-bvanassche@acm.org (mailing list archive)
State New, archived
Headers show
Series Pass data lifetime information to SCSI disk devices | expand

Commit Message

Bart Van Assche Nov. 14, 2023, 9:40 p.m. UTC
Prepare for supporting more data lifetimes by changing data lifetime
names into numeric constants.

Cc: Kanchan Joshi <joshi.k@samsung.com>
Cc: Jan Kara <jack@suse.cz>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Christian Brauner <brauner@kernel.org>
Suggested-by: Kanchan Joshi <joshi.k@samsung.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 fs/f2fs/segment.c  |  4 ++--
 fs/inode.c         |  2 +-
 include/linux/fs.h | 12 ++++++------
 3 files changed, 9 insertions(+), 9 deletions(-)

Comments

Kanchan Joshi Nov. 20, 2023, 7:19 a.m. UTC | #1
On 11/15/2023 3:10 AM, Bart Van Assche wrote:

Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>
Christoph Hellwig Nov. 27, 2023, 7:08 a.m. UTC | #2
On Tue, Nov 14, 2023 at 01:40:56PM -0800, Bart Van Assche wrote:
> -	case WRITE_LIFE_SHORT:
> +	case WRITE_LIFE_2:
>  		return CURSEG_HOT_DATA;
> -	case WRITE_LIFE_EXTREME:
> +	case WRITE_LIFE_5:
>  		return CURSEG_COLD_DATA;
>  	default:
>  		return CURSEG_WARM_DATA;

A WRITE_LIFE_2 constant is strictly more confusing than just using 2,
so this patch makes no sense whatsoever.

More importantly these constant have been around forever, so we'd better
have a really good argument for changing them.
Kanchan Joshi Nov. 27, 2023, 8:45 a.m. UTC | #3
On 11/27/2023 12:38 PM, Christoph Hellwig wrote:
> On Tue, Nov 14, 2023 at 01:40:56PM -0800, Bart Van Assche wrote:
>> -	case WRITE_LIFE_SHORT:
>> +	case WRITE_LIFE_2:
>>   		return CURSEG_HOT_DATA;
>> -	case WRITE_LIFE_EXTREME:
>> +	case WRITE_LIFE_5:
>>   		return CURSEG_COLD_DATA;
>>   	default:
>>   		return CURSEG_WARM_DATA;
> 
> A WRITE_LIFE_2 constant is strictly more confusing than just using 2,
> so this patch makes no sense whatsoever.
> 
> More importantly these constant have been around forever, so we'd better
> have a really good argument for changing them.

How about this argument (assuming you may not have seen) from previous 
iteration [1]-

"Does it make sense to do away with these, and have temperature-neutral
names instead e.g., WRITE_LIFE_1, WRITE_LIFE_2?

With the current choice:
- If the count goes up (beyond 5 hints), infra can scale fine but these
names do not. Imagine ULTRA_EXTREME after EXTREME.
- Applications or in-kernel users can specify LONG hint with data that
actually has a SHORT lifetime. Nothing really ensures that LONG is
really LONG.

Temperature-neutral names seem more generic/scalable and do not present
the unnecessary need to be accurate with relative temperatures."

[1]
https://lore.kernel.org/linux-block/b3058ce6-e297-b4c3-71d4-4b76f76439ba@samsung.com/
Christoph Hellwig Nov. 27, 2023, 9:29 a.m. UTC | #4
On Mon, Nov 27, 2023 at 02:15:51PM +0530, Kanchan Joshi wrote:
> How about this argument (assuming you may not have seen) from previous 
> iteration [1]-
> 
> "Does it make sense to do away with these, and have temperature-neutral
> names instead e.g., WRITE_LIFE_1, WRITE_LIFE_2?
> 
> With the current choice:
> - If the count goes up (beyond 5 hints), infra can scale fine but these
> names do not. Imagine ULTRA_EXTREME after EXTREME.
> - Applications or in-kernel users can specify LONG hint with data that
> actually has a SHORT lifetime. Nothing really ensures that LONG is
> really LONG.
> 
> Temperature-neutral names seem more generic/scalable and do not present
> the unnecessary need to be accurate with relative temperatures."

I don't really buy it, as that's not the use case we currently have,
which hasn't changed.  And even if we did, life would probably be
simpler if you decoupled it from this series..

But IFF we decided to do away with the meanings, having constants
that just are numbered simply doesn't make any sense.
Bart Van Assche Nov. 27, 2023, 7 p.m. UTC | #5
On 11/26/23 23:08, Christoph Hellwig wrote:
> More importantly these constant have been around forever, so we'd better
> have a really good argument for changing them.

Hi Christoph,

I will drop this patch.

As you know the NVMe and SCSI specifications use the numeric range 0..63 for
the data lifetime so there is a gap between the values supported by the
F_[GS]ET_RW_HINT fcntls and the data lifetime values accepted by widely used
storage devices. Do you think that it should be possible for user space
applications to specify the full range (0..63)?

Thanks,

Bart.
diff mbox series

Patch

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 727d016318f9..098a574d8d84 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -3281,9 +3281,9 @@  int f2fs_trim_fs(struct f2fs_sb_info *sbi, struct fstrim_range *range)
 int f2fs_rw_hint_to_seg_type(enum rw_hint hint)
 {
 	switch (hint) {
-	case WRITE_LIFE_SHORT:
+	case WRITE_LIFE_2:
 		return CURSEG_HOT_DATA;
-	case WRITE_LIFE_EXTREME:
+	case WRITE_LIFE_5:
 		return CURSEG_COLD_DATA;
 	default:
 		return CURSEG_WARM_DATA;
diff --git a/fs/inode.c b/fs/inode.c
index edcd8a61975f..7965d5e07012 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -175,7 +175,7 @@  int inode_init_always(struct super_block *sb, struct inode *inode)
 	i_gid_write(inode, 0);
 	atomic_set(&inode->i_writecount, 0);
 	inode->i_size = 0;
-	inode->i_write_hint = WRITE_LIFE_NOT_SET;
+	inode->i_write_hint = WRITE_LIFE_0;
 	inode->i_blocks = 0;
 	inode->i_bytes = 0;
 	inode->i_generation = 0;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 98b7a7a8c42e..59f9de9df0fe 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -314,12 +314,12 @@  struct readahead_control;
  * Stored in struct inode as u8.
  */
 enum rw_hint {
-	WRITE_LIFE_NOT_SET	= 0,
-	WRITE_LIFE_NONE		= RWH_WRITE_LIFE_NONE,
-	WRITE_LIFE_SHORT	= RWH_WRITE_LIFE_SHORT,
-	WRITE_LIFE_MEDIUM	= RWH_WRITE_LIFE_MEDIUM,
-	WRITE_LIFE_LONG		= RWH_WRITE_LIFE_LONG,
-	WRITE_LIFE_EXTREME	= RWH_WRITE_LIFE_EXTREME,
+	WRITE_LIFE_0	= 0,
+	WRITE_LIFE_1	= RWH_WRITE_LIFE_NONE,
+	WRITE_LIFE_2	= RWH_WRITE_LIFE_SHORT,
+	WRITE_LIFE_3	= RWH_WRITE_LIFE_MEDIUM,
+	WRITE_LIFE_4	= RWH_WRITE_LIFE_LONG,
+	WRITE_LIFE_5	= RWH_WRITE_LIFE_EXTREME,
 };
 
 /* Match RWF_* bits to IOCB bits */