| Message ID | 20240910150200.6589-2-joshi.k@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v5,1/5] fs, block: refactor enum rw_hint | expand |
On Tue, Sep 10, 2024 at 08:31:56PM +0530, Kanchan Joshi wrote: > Rename enum rw_hint to rw_lifetime_hint. > Change i_write_hint (in inode), bi_write_hint(in bio), and write_hint > (in request) to use u8 data-type rather than this enum. > > This is in preparation to introduce a new write hint type. The rationale seems a bit sparse. Why is it renamed? Because the name fits better, because you need the same for something else? > static void submit_bh_wbc(blk_opf_t opf, struct buffer_head *bh, > - enum rw_hint hint, struct writeback_control *wbc); > + u8 hint, struct writeback_control *wbc); And moving from the enum to an plain integer seems like a bit of a retrograde step.
On 9/12/2024 6:23 PM, Christoph Hellwig wrote: > On Tue, Sep 10, 2024 at 08:31:56PM +0530, Kanchan Joshi wrote: >> Rename enum rw_hint to rw_lifetime_hint. >> Change i_write_hint (in inode), bi_write_hint(in bio), and write_hint >> (in request) to use u8 data-type rather than this enum. >> >> This is in preparation to introduce a new write hint type. > > The rationale seems a bit sparse. Why is it renamed? Because the > name fits better, because you need the same for something else? > Right, new name fits better. Because 'enum rw_hint' is a generic name that conveys 'any' hint. This was fine before. But once we start supporting more than one hint type, we need to be specific what hint-type is being handled. More below. >> static void submit_bh_wbc(blk_opf_t opf, struct buffer_head *bh, >> - enum rw_hint hint, struct writeback_control *wbc); >> + u8 hint, struct writeback_control *wbc); > > And moving from the enum to an plain integer seems like a bit of a > retrograde step. This particular enum is hardwired to take 6 temperature-hint values [*]. But this (and many other) functions act as a simple propagator, which do not have to care whether hint type is lifetime or placement or anything else. The creator/originator of the hint decides what hint to pass (userspace in this case). And the consumer (driver in this case) decides whether or not it understands the hint that has been passed. The intermediate components/functions only need to pass the hint, regardless of its type, down. Wherever hint is being used in generic way, u8 data type is being used. Down the line if a component/function needs to care for a specific type, it can start decoding the passed hint type/value (using the appropriate macro similar to what this series does for SCSI and NVMe). Overall, this also helps to avoid the churn. Otherwise we duplicate all the propagation code that has been done for temperature hint across the IO stack. [*] enum rw_hint { WRITE_LIFE_NOT_SET = RWH_WRITE_LIFE_NOT_SET, 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, } __packed;
On 9/12/24 8:50 AM, Kanchan Joshi wrote:
> Wherever hint is being used in generic way, u8 data type is being used.
Has it been considered to introduce a new union and to use that as the
type of 'hint' instead of 'u8'?
Thanks,
Bart.
On 9/13/2024 2:00 AM, Bart Van Assche wrote: > On 9/12/24 8:50 AM, Kanchan Joshi wrote: >> Wherever hint is being used in generic way, u8 data type is being used. > > Has it been considered to introduce a new union and to use that as the > type of 'hint' instead of 'u8'? > Is it same as your other question in patch 3?. I commented there. If not, can you expand on what you prefer (maybe with a code fragment).
diff --git a/fs/buffer.c b/fs/buffer.c index e55ad471c530..0c6bc9b7d4ad 100644 --- a/fs/buffer.c +++ b/fs/buffer.c @@ -55,7 +55,7 @@ static int fsync_buffers_list(spinlock_t *lock, struct list_head *list); static void submit_bh_wbc(blk_opf_t opf, struct buffer_head *bh, - enum rw_hint hint, struct writeback_control *wbc); + u8 hint, struct writeback_control *wbc); #define BH_ENTRY(list) list_entry((list), struct buffer_head, b_assoc_buffers) @@ -2778,7 +2778,7 @@ static void end_bio_bh_io_sync(struct bio *bio) } static void submit_bh_wbc(blk_opf_t opf, struct buffer_head *bh, - enum rw_hint write_hint, + u8 write_hint, struct writeback_control *wbc) { const enum req_op op = opf & REQ_OP_MASK; diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index ac19c61f0c3e..9b843b57dba1 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -3756,8 +3756,9 @@ int f2fs_build_segment_manager(struct f2fs_sb_info *sbi); void f2fs_destroy_segment_manager(struct f2fs_sb_info *sbi); int __init f2fs_create_segment_manager_caches(void); void f2fs_destroy_segment_manager_caches(void); -int f2fs_rw_hint_to_seg_type(struct f2fs_sb_info *sbi, enum rw_hint hint); -enum rw_hint f2fs_io_type_to_rw_hint(struct f2fs_sb_info *sbi, +int f2fs_rw_hint_to_seg_type(struct f2fs_sb_info *sbi, + enum rw_lifetime_hint hint); +enum rw_lifetime_hint f2fs_io_type_to_rw_hint(struct f2fs_sb_info *sbi, enum page_type type, enum temp_type temp); unsigned int f2fs_usable_segs_in_sec(struct f2fs_sb_info *sbi, unsigned int segno); diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c index 78c3198a6308..6802e82f9ffd 100644 --- a/fs/f2fs/segment.c +++ b/fs/f2fs/segment.c @@ -3381,7 +3381,8 @@ int f2fs_trim_fs(struct f2fs_sb_info *sbi, struct fstrim_range *range) return err; } -int f2fs_rw_hint_to_seg_type(struct f2fs_sb_info *sbi, enum rw_hint hint) +int f2fs_rw_hint_to_seg_type(struct f2fs_sb_info *sbi, + enum rw_lifetime_hint hint) { if (F2FS_OPTION(sbi).active_logs == 2) return CURSEG_HOT_DATA; @@ -3425,7 +3426,7 @@ int f2fs_rw_hint_to_seg_type(struct f2fs_sb_info *sbi, enum rw_hint hint) * WRITE_LIFE_MEDIUM " WRITE_LIFE_MEDIUM * WRITE_LIFE_LONG " WRITE_LIFE_LONG */ -enum rw_hint f2fs_io_type_to_rw_hint(struct f2fs_sb_info *sbi, +enum rw_lifetime_hint f2fs_io_type_to_rw_hint(struct f2fs_sb_info *sbi, enum page_type type, enum temp_type temp) { switch (type) { diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h index 8d304b1d16b1..1e5ce84ccf0a 100644 --- a/include/linux/blk-mq.h +++ b/include/linux/blk-mq.h @@ -159,7 +159,7 @@ struct request { struct blk_crypto_keyslot *crypt_keyslot; #endif - enum rw_hint write_hint; + u8 write_hint; unsigned short ioprio; enum mq_rq_state state; diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h index 36ed96133217..446c847bb3b3 100644 --- a/include/linux/blk_types.h +++ b/include/linux/blk_types.h @@ -216,7 +216,7 @@ struct bio { */ unsigned short bi_flags; /* BIO_* below */ unsigned short bi_ioprio; - enum rw_hint bi_write_hint; + u8 bi_write_hint; blk_status_t bi_status; atomic_t __bi_remaining; diff --git a/include/linux/fs.h b/include/linux/fs.h index fb0426f349fc..f9a7a2a80661 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -674,7 +674,7 @@ struct inode { spinlock_t i_lock; /* i_blocks, i_bytes, maybe i_size */ unsigned short i_bytes; u8 i_blkbits; - enum rw_hint i_write_hint; + u8 i_write_hint; blkcnt_t i_blocks; #ifdef __NEED_I_SIZE_ORDERED diff --git a/include/linux/rw_hint.h b/include/linux/rw_hint.h index 309ca72f2dfb..b9942f5f13d3 100644 --- a/include/linux/rw_hint.h +++ b/include/linux/rw_hint.h @@ -7,7 +7,7 @@ #include <uapi/linux/fcntl.h> /* Block storage write lifetime hint values. */ -enum rw_hint { +enum rw_lifetime_hint { WRITE_LIFE_NOT_SET = RWH_WRITE_LIFE_NOT_SET, WRITE_LIFE_NONE = RWH_WRITE_LIFE_NONE, WRITE_LIFE_SHORT = RWH_WRITE_LIFE_SHORT, @@ -18,7 +18,7 @@ enum rw_hint { /* Sparse ignores __packed annotations on enums, hence the #ifndef below. */ #ifndef __CHECKER__ -static_assert(sizeof(enum rw_hint) == 1); +static_assert(sizeof(enum rw_lifetime_hint) == 1); #endif #endif /* _LINUX_RW_HINT_H */
Rename enum rw_hint to rw_lifetime_hint. Change i_write_hint (in inode), bi_write_hint(in bio), and write_hint (in request) to use u8 data-type rather than this enum. This is in preparation to introduce a new write hint type. Signed-off-by: Kanchan Joshi <joshi.k@samsung.com> --- fs/buffer.c | 4 ++-- fs/f2fs/f2fs.h | 5 +++-- fs/f2fs/segment.c | 5 +++-- include/linux/blk-mq.h | 2 +- include/linux/blk_types.h | 2 +- include/linux/fs.h | 2 +- include/linux/rw_hint.h | 4 ++-- 7 files changed, 13 insertions(+), 11 deletions(-)