| Message ID | 20231005194129.1882245-5-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Pass data temperature information to UFS devices | expand |
On 10/6/2023 1:10 AM, Bart Van Assche wrote: > This patch reverts a small subset of commit c75e707fe1aa ("block: remove > the per-bio/request write hint"). The following functionality has been > restored: > - In F2FS, store data lifetime information in struct bio. > - In fs/iomap and fs/mpage.c, restore the code that sets the data > lifetime. > > A new header file is introduced for the new bio_[sg]et_data_lifetime() > functions because there is no other header file yet that includes both > <linux/fs.h> and <linux/ioprio.h>. > > The value WRITE_LIFE_NONE is mapped onto the data lifetime 0. This is > consistent with NVMe TPAR4093a. From that TPAR: "A value of 1h specifies > the shortest Data Lifetime. A value of 3Fh specifies the longest Data > Lifetime." This is also consistent with the SCSI specifications. From > T10 document 23-024r3: "0h: no relative lifetime is applicable; 1h: > shortest relative lifetime; ...; 3fh: longest relative lifetime". > > Cc: Christoph Hellwig <hch@lst.de> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > fs/f2fs/data.c | 3 +++ > fs/iomap/buffered-io.c | 3 +++ > fs/mpage.c | 2 ++ > include/linux/fs-lifetime.h | 20 ++++++++++++++++++++ > 4 files changed, 28 insertions(+) > create mode 100644 include/linux/fs-lifetime.h > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index 916e317ac925..2962cb335897 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -6,6 +6,7 @@ > * http://www.samsung.com/ > */ > #include <linux/fs.h> > +#include <linux/fs-lifetime.h> > #include <linux/f2fs_fs.h> > #include <linux/buffer_head.h> > #include <linux/sched/mm.h> > @@ -478,6 +479,8 @@ static struct bio *__bio_alloc(struct f2fs_io_info *fio, int npages) > } else { > bio->bi_end_io = f2fs_write_end_io; > bio->bi_private = sbi; > + bio_set_data_lifetime(bio, > + f2fs_io_type_to_rw_hint(sbi, fio->type, fio->temp)); > } > iostat_alloc_and_bind_ctx(sbi, bio, NULL); > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index 644479ccefbd..9bf05342ca65 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -6,6 +6,7 @@ > #include <linux/module.h> > #include <linux/compiler.h> > #include <linux/fs.h> > +#include <linux/fs-lifetime.h> > #include <linux/iomap.h> > #include <linux/pagemap.h> > #include <linux/uio.h> > @@ -1660,6 +1661,7 @@ iomap_alloc_ioend(struct inode *inode, struct iomap_writepage_ctx *wpc, > REQ_OP_WRITE | wbc_to_write_flags(wbc), > GFP_NOFS, &iomap_ioend_bioset); > bio->bi_iter.bi_sector = sector; > + bio_set_data_lifetime(bio, inode->i_write_hint); > wbc_init_bio(wbc, bio); > > ioend = container_of(bio, struct iomap_ioend, io_inline_bio); > @@ -1690,6 +1692,7 @@ iomap_chain_bio(struct bio *prev) > new = bio_alloc(prev->bi_bdev, BIO_MAX_VECS, prev->bi_opf, GFP_NOFS); > bio_clone_blkg_association(new, prev); > new->bi_iter.bi_sector = bio_end_sector(prev); > + bio_set_data_lifetime(new, bio_get_data_lifetime(prev)); > > bio_chain(prev, new); > bio_get(prev); /* for iomap_finish_ioend */ > diff --git a/fs/mpage.c b/fs/mpage.c > index 242e213ee064..888ca71c9ea7 100644 > --- a/fs/mpage.c > +++ b/fs/mpage.c > @@ -20,6 +20,7 @@ > #include <linux/gfp.h> > #include <linux/bio.h> > #include <linux/fs.h> > +#include <linux/fs-lifetime.h> > #include <linux/buffer_head.h> > #include <linux/blkdev.h> > #include <linux/highmem.h> > @@ -612,6 +613,7 @@ static int __mpage_writepage(struct folio *folio, struct writeback_control *wbc, > GFP_NOFS); > bio->bi_iter.bi_sector = blocks[0] << (blkbits - 9); > wbc_init_bio(wbc, bio); > + bio_set_data_lifetime(bio, inode->i_write_hint); > } > > /* > diff --git a/include/linux/fs-lifetime.h b/include/linux/fs-lifetime.h > new file mode 100644 > index 000000000000..0e652e00cfab > --- /dev/null > +++ b/include/linux/fs-lifetime.h > @@ -0,0 +1,20 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#include <linux/bio.h> > +#include <linux/fs.h> > +#include <linux/ioprio.h> > + > +static inline enum rw_hint bio_get_data_lifetime(struct bio *bio) > +{ > + /* +1 to map 0 onto WRITE_LIFE_NONE. */ > + return IOPRIO_PRIO_LIFETIME(bio->bi_ioprio) + 1; > +} > + > +static inline void bio_set_data_lifetime(struct bio *bio, enum rw_hint lifetime) > +{ > + /* -1 to map WRITE_LIFE_NONE onto 0. */ > + if (lifetime != 0) > + lifetime--; How the driver can figure when lifetime is not set, and when it is set to WRITE_LIFE_NONE? If it uses IOPRIO_PRIO_LIFETIME (as patch 8 does), it will see 0 in both cases. F2FS fs-based whint_mode seems to expect distinct streams for WRITE_LIFE_NOT_SET and WRITE_LIFE_NONE.
On 10/9/23 22:42, Kanchan Joshi wrote: > On 10/6/2023 1:10 AM, Bart Van Assche wrote: >> +static inline enum rw_hint bio_get_data_lifetime(struct bio *bio) >> +{ >> + /* +1 to map 0 onto WRITE_LIFE_NONE. */ >> + return IOPRIO_PRIO_LIFETIME(bio->bi_ioprio) + 1; >> +} >> + >> +static inline void bio_set_data_lifetime(struct bio *bio, enum rw_hint lifetime) >> +{ >> + /* -1 to map WRITE_LIFE_NONE onto 0. */ >> + if (lifetime != 0) >> + lifetime--; > > How the driver can figure when lifetime is not set, and when it is set > to WRITE_LIFE_NONE? If it uses IOPRIO_PRIO_LIFETIME (as patch 8 does), > it will see 0 in both cases. > F2FS fs-based whint_mode seems to expect distinct streams for > WRITE_LIFE_NOT_SET and WRITE_LIFE_NONE. I will remove the -1 / +1 from the above code. Thanks, Bart.
On Thu, Oct 05, 2023 at 12:40:50PM -0700, Bart Van Assche wrote: > This patch reverts a small subset of commit c75e707fe1aa ("block: remove > the per-bio/request write hint"). The following functionality has been > restored: Please explain this in terms of what you add. The fact that it restores something isn't more than a little footnote added at the end. > --- /dev/null > +++ b/include/linux/fs-lifetime.h The name seems a bit odd for something that primarily deals with bios. bio-lifetime.h would seem like a better fit. > @@ -0,0 +1,20 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#include <linux/bio.h> > +#include <linux/fs.h> > +#include <linux/ioprio.h> > + > +static inline enum rw_hint bio_get_data_lifetime(struct bio *bio) > +{ > + /* +1 to map 0 onto WRITE_LIFE_NONE. */ > + return IOPRIO_PRIO_LIFETIME(bio->bi_ioprio) + 1; This seems a little to magic. Why not a lookup table? > +} > + > +static inline void bio_set_data_lifetime(struct bio *bio, enum rw_hint lifetime) Please avoid the overly long line. > + /* -1 to map WRITE_LIFE_NONE onto 0. */ > + if (lifetime != 0) > + lifetime--; > + WARN_ON_ONCE(lifetime & ~IOPRIO_LIFETIME_MASK); I'd return here instead of propagating the bogus value.
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index 916e317ac925..2962cb335897 100644 --- a/fs/f2fs/data.c +++ b/fs/f2fs/data.c @@ -6,6 +6,7 @@ * http://www.samsung.com/ */ #include <linux/fs.h> +#include <linux/fs-lifetime.h> #include <linux/f2fs_fs.h> #include <linux/buffer_head.h> #include <linux/sched/mm.h> @@ -478,6 +479,8 @@ static struct bio *__bio_alloc(struct f2fs_io_info *fio, int npages) } else { bio->bi_end_io = f2fs_write_end_io; bio->bi_private = sbi; + bio_set_data_lifetime(bio, + f2fs_io_type_to_rw_hint(sbi, fio->type, fio->temp)); } iostat_alloc_and_bind_ctx(sbi, bio, NULL); diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index 644479ccefbd..9bf05342ca65 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -6,6 +6,7 @@ #include <linux/module.h> #include <linux/compiler.h> #include <linux/fs.h> +#include <linux/fs-lifetime.h> #include <linux/iomap.h> #include <linux/pagemap.h> #include <linux/uio.h> @@ -1660,6 +1661,7 @@ iomap_alloc_ioend(struct inode *inode, struct iomap_writepage_ctx *wpc, REQ_OP_WRITE | wbc_to_write_flags(wbc), GFP_NOFS, &iomap_ioend_bioset); bio->bi_iter.bi_sector = sector; + bio_set_data_lifetime(bio, inode->i_write_hint); wbc_init_bio(wbc, bio); ioend = container_of(bio, struct iomap_ioend, io_inline_bio); @@ -1690,6 +1692,7 @@ iomap_chain_bio(struct bio *prev) new = bio_alloc(prev->bi_bdev, BIO_MAX_VECS, prev->bi_opf, GFP_NOFS); bio_clone_blkg_association(new, prev); new->bi_iter.bi_sector = bio_end_sector(prev); + bio_set_data_lifetime(new, bio_get_data_lifetime(prev)); bio_chain(prev, new); bio_get(prev); /* for iomap_finish_ioend */ diff --git a/fs/mpage.c b/fs/mpage.c index 242e213ee064..888ca71c9ea7 100644 --- a/fs/mpage.c +++ b/fs/mpage.c @@ -20,6 +20,7 @@ #include <linux/gfp.h> #include <linux/bio.h> #include <linux/fs.h> +#include <linux/fs-lifetime.h> #include <linux/buffer_head.h> #include <linux/blkdev.h> #include <linux/highmem.h> @@ -612,6 +613,7 @@ static int __mpage_writepage(struct folio *folio, struct writeback_control *wbc, GFP_NOFS); bio->bi_iter.bi_sector = blocks[0] << (blkbits - 9); wbc_init_bio(wbc, bio); + bio_set_data_lifetime(bio, inode->i_write_hint); } /* diff --git a/include/linux/fs-lifetime.h b/include/linux/fs-lifetime.h new file mode 100644 index 000000000000..0e652e00cfab --- /dev/null +++ b/include/linux/fs-lifetime.h @@ -0,0 +1,20 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#include <linux/bio.h> +#include <linux/fs.h> +#include <linux/ioprio.h> + +static inline enum rw_hint bio_get_data_lifetime(struct bio *bio) +{ + /* +1 to map 0 onto WRITE_LIFE_NONE. */ + return IOPRIO_PRIO_LIFETIME(bio->bi_ioprio) + 1; +} + +static inline void bio_set_data_lifetime(struct bio *bio, enum rw_hint lifetime) +{ + /* -1 to map WRITE_LIFE_NONE onto 0. */ + if (lifetime != 0) + lifetime--; + WARN_ON_ONCE(lifetime & ~IOPRIO_LIFETIME_MASK); + bio->bi_ioprio &= ~(IOPRIO_LIFETIME_MASK << IOPRIO_LIFETIME_SHIFT); + bio->bi_ioprio |= lifetime << IOPRIO_LIFETIME_SHIFT; +}
This patch reverts a small subset of commit c75e707fe1aa ("block: remove the per-bio/request write hint"). The following functionality has been restored: - In F2FS, store data lifetime information in struct bio. - In fs/iomap and fs/mpage.c, restore the code that sets the data lifetime. A new header file is introduced for the new bio_[sg]et_data_lifetime() functions because there is no other header file yet that includes both <linux/fs.h> and <linux/ioprio.h>. The value WRITE_LIFE_NONE is mapped onto the data lifetime 0. This is consistent with NVMe TPAR4093a. From that TPAR: "A value of 1h specifies the shortest Data Lifetime. A value of 3Fh specifies the longest Data Lifetime." This is also consistent with the SCSI specifications. From T10 document 23-024r3: "0h: no relative lifetime is applicable; 1h: shortest relative lifetime; ...; 3fh: longest relative lifetime". Cc: Christoph Hellwig <hch@lst.de> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- fs/f2fs/data.c | 3 +++ fs/iomap/buffered-io.c | 3 +++ fs/mpage.c | 2 ++ include/linux/fs-lifetime.h | 20 ++++++++++++++++++++ 4 files changed, 28 insertions(+) create mode 100644 include/linux/fs-lifetime.h