| 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 |
On 11/15/2023 3:10 AM, Bart Van Assche wrote:
Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>
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.
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/
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.
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 --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 */
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(-)