| Message ID | 20190218100433.20048-7-chandan@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Consolidate Post read processing code | expand |
Hi Chandan, On Mon, Feb 18, 2019 at 03:34:29PM +0530, Chandan Rajendra wrote: > Ext4 and F2FS currently use a non-NULL value stored at bio->bi_private > to determine if the contents of the bio need to be "post processed" > i.e. whether its contents need to be decrypted and/or verified. For > block size < page size scenario, bio->bi_private would hold a pointer to > buffer_head. Hence, this commit adds the new flag REQ_POST_READ_PROC to > be able to decisively check for post process requirement for a bio. > > Signed-off-by: Chandan Rajendra <chandan@linux.ibm.com> > --- > fs/ext4/readpage.c | 11 +++++++++-- > fs/post_read_process.c | 2 +- > include/linux/blk_types.h | 2 ++ > 3 files changed, 12 insertions(+), 3 deletions(-) > > diff --git a/fs/ext4/readpage.c b/fs/ext4/readpage.c > index 8943fc41fd33..c7dbab35deaa 100644 > --- a/fs/ext4/readpage.c > +++ b/fs/ext4/readpage.c > @@ -245,6 +245,7 @@ int ext4_mpage_readpages(struct address_space *mapping, > } > if (bio == NULL) { > struct bio_post_read_ctx *ctx; > + unsigned int op_flags = 0; > > bio = bio_alloc(GFP_KERNEL, > min_t(int, nr_pages, BIO_MAX_PAGES)); > @@ -259,8 +260,14 @@ int ext4_mpage_readpages(struct address_space *mapping, > bio->bi_iter.bi_sector = blocks[0] << (blkbits - 9); > bio->bi_end_io = mpage_end_io; > bio->bi_private = ctx; > - bio_set_op_attrs(bio, REQ_OP_READ, > - is_readahead ? REQ_RAHEAD : 0); > + > + if (is_readahead) > + op_flags |= REQ_RAHEAD; > + > + if (ctx) > + op_flags |= REQ_POST_READ_PROC; > + > + bio_set_op_attrs(bio, REQ_OP_READ, op_flags); > } > > length = first_hole << blkbits; > diff --git a/fs/post_read_process.c b/fs/post_read_process.c > index 1f8663d70247..66c1c6e57e70 100644 > --- a/fs/post_read_process.c > +++ b/fs/post_read_process.c > @@ -104,7 +104,7 @@ void put_bio_post_read_ctx(struct bio_post_read_ctx *ctx) > > bool bio_post_read_required(struct bio *bio) > { > - return bio->bi_private && !bio->bi_status; > + return bio->bi_opf & REQ_POST_READ_PROC; > } > > static int __init bio_init_post_read_processing(void) > diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h > index 5c7e7f859a24..6904945c8c40 100644 > --- a/include/linux/blk_types.h > +++ b/include/linux/blk_types.h > @@ -320,6 +320,7 @@ enum req_flag_bits { > __REQ_RAHEAD, /* read ahead, can fail anytime */ > __REQ_BACKGROUND, /* background IO */ > __REQ_NOWAIT, /* Don't wait if request will block */ > + __REQ_POST_READ_PROC, > > /* command specific flags for REQ_OP_WRITE_ZEROES: */ > __REQ_NOUNMAP, /* do not free blocks when zeroing */ > @@ -346,6 +347,7 @@ enum req_flag_bits { > #define REQ_RAHEAD (1ULL << __REQ_RAHEAD) > #define REQ_BACKGROUND (1ULL << __REQ_BACKGROUND) > #define REQ_NOWAIT (1ULL << __REQ_NOWAIT) > +#define REQ_POST_READ_PROC (1ULL << __REQ_POST_READ_PROC) > #define REQ_NOUNMAP (1ULL << __REQ_NOUNMAP) > #define REQ_HIPRI (1ULL << __REQ_HIPRI) > > -- > 2.19.1 > > > I don't think this is an appropriate use of a request flag, as request flags are meant for the block layer. Also doesn't the bio still need a pointer to the bio_post_read_ctx anyway? So I don't see how this would solve the problem, if ->bi_private is already used. - Eric
On Wednesday, February 20, 2019 5:51:00 AM IST Eric Biggers wrote: > Hi Chandan, > > On Mon, Feb 18, 2019 at 03:34:29PM +0530, Chandan Rajendra wrote: > > Ext4 and F2FS currently use a non-NULL value stored at bio->bi_private > > to determine if the contents of the bio need to be "post processed" > > i.e. whether its contents need to be decrypted and/or verified. For > > block size < page size scenario, bio->bi_private would hold a pointer to > > buffer_head. Hence, this commit adds the new flag REQ_POST_READ_PROC to > > be able to decisively check for post process requirement for a bio. > > > > Signed-off-by: Chandan Rajendra <chandan@linux.ibm.com> > > --- > > fs/ext4/readpage.c | 11 +++++++++-- > > fs/post_read_process.c | 2 +- > > include/linux/blk_types.h | 2 ++ > > 3 files changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/fs/ext4/readpage.c b/fs/ext4/readpage.c > > index 8943fc41fd33..c7dbab35deaa 100644 > > --- a/fs/ext4/readpage.c > > +++ b/fs/ext4/readpage.c > > @@ -245,6 +245,7 @@ int ext4_mpage_readpages(struct address_space *mapping, > > } > > if (bio == NULL) { > > struct bio_post_read_ctx *ctx; > > + unsigned int op_flags = 0; > > > > bio = bio_alloc(GFP_KERNEL, > > min_t(int, nr_pages, BIO_MAX_PAGES)); > > @@ -259,8 +260,14 @@ int ext4_mpage_readpages(struct address_space *mapping, > > bio->bi_iter.bi_sector = blocks[0] << (blkbits - 9); > > bio->bi_end_io = mpage_end_io; > > bio->bi_private = ctx; > > - bio_set_op_attrs(bio, REQ_OP_READ, > > - is_readahead ? REQ_RAHEAD : 0); > > + > > + if (is_readahead) > > + op_flags |= REQ_RAHEAD; > > + > > + if (ctx) > > + op_flags |= REQ_POST_READ_PROC; > > + > > + bio_set_op_attrs(bio, REQ_OP_READ, op_flags); > > } > > > > length = first_hole << blkbits; > > diff --git a/fs/post_read_process.c b/fs/post_read_process.c > > index 1f8663d70247..66c1c6e57e70 100644 > > --- a/fs/post_read_process.c > > +++ b/fs/post_read_process.c > > @@ -104,7 +104,7 @@ void put_bio_post_read_ctx(struct bio_post_read_ctx *ctx) > > > > bool bio_post_read_required(struct bio *bio) > > { > > - return bio->bi_private && !bio->bi_status; > > + return bio->bi_opf & REQ_POST_READ_PROC; > > } > > > > static int __init bio_init_post_read_processing(void) > > diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h > > index 5c7e7f859a24..6904945c8c40 100644 > > --- a/include/linux/blk_types.h > > +++ b/include/linux/blk_types.h > > @@ -320,6 +320,7 @@ enum req_flag_bits { > > __REQ_RAHEAD, /* read ahead, can fail anytime */ > > __REQ_BACKGROUND, /* background IO */ > > __REQ_NOWAIT, /* Don't wait if request will block */ > > + __REQ_POST_READ_PROC, > > > > /* command specific flags for REQ_OP_WRITE_ZEROES: */ > > __REQ_NOUNMAP, /* do not free blocks when zeroing */ > > @@ -346,6 +347,7 @@ enum req_flag_bits { > > #define REQ_RAHEAD (1ULL << __REQ_RAHEAD) > > #define REQ_BACKGROUND (1ULL << __REQ_BACKGROUND) > > #define REQ_NOWAIT (1ULL << __REQ_NOWAIT) > > +#define REQ_POST_READ_PROC (1ULL << __REQ_POST_READ_PROC) > > #define REQ_NOUNMAP (1ULL << __REQ_NOUNMAP) > > #define REQ_HIPRI (1ULL << __REQ_HIPRI) > > > > I don't think this is an appropriate use of a request flag, as request flags are > meant for the block layer. > > Also doesn't the bio still need a pointer to the bio_post_read_ctx anyway? So I > don't see how this would solve the problem, if ->bi_private is already used. I had glanced across block_read_full_page() function which implements reading non-contiguous blocks mapped by the page in block size < page size scenario. Here bio->bi_private would point to the buffer head that represents the block on which read I/O was performed. In such a case, bio_post_read_required() would always return true. Hence I decided to add this request flag. Now, I believe we can actually save away the ctx pointer in bh->b_private member and later decide on whether the buffer head requires post processing based on bh->bi_private's non-NULL value. I will have to read up the code more thoroughly to confirm this.
diff --git a/fs/ext4/readpage.c b/fs/ext4/readpage.c index 8943fc41fd33..c7dbab35deaa 100644 --- a/fs/ext4/readpage.c +++ b/fs/ext4/readpage.c @@ -245,6 +245,7 @@ int ext4_mpage_readpages(struct address_space *mapping, } if (bio == NULL) { struct bio_post_read_ctx *ctx; + unsigned int op_flags = 0; bio = bio_alloc(GFP_KERNEL, min_t(int, nr_pages, BIO_MAX_PAGES)); @@ -259,8 +260,14 @@ int ext4_mpage_readpages(struct address_space *mapping, bio->bi_iter.bi_sector = blocks[0] << (blkbits - 9); bio->bi_end_io = mpage_end_io; bio->bi_private = ctx; - bio_set_op_attrs(bio, REQ_OP_READ, - is_readahead ? REQ_RAHEAD : 0); + + if (is_readahead) + op_flags |= REQ_RAHEAD; + + if (ctx) + op_flags |= REQ_POST_READ_PROC; + + bio_set_op_attrs(bio, REQ_OP_READ, op_flags); } length = first_hole << blkbits; diff --git a/fs/post_read_process.c b/fs/post_read_process.c index 1f8663d70247..66c1c6e57e70 100644 --- a/fs/post_read_process.c +++ b/fs/post_read_process.c @@ -104,7 +104,7 @@ void put_bio_post_read_ctx(struct bio_post_read_ctx *ctx) bool bio_post_read_required(struct bio *bio) { - return bio->bi_private && !bio->bi_status; + return bio->bi_opf & REQ_POST_READ_PROC; } static int __init bio_init_post_read_processing(void) diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h index 5c7e7f859a24..6904945c8c40 100644 --- a/include/linux/blk_types.h +++ b/include/linux/blk_types.h @@ -320,6 +320,7 @@ enum req_flag_bits { __REQ_RAHEAD, /* read ahead, can fail anytime */ __REQ_BACKGROUND, /* background IO */ __REQ_NOWAIT, /* Don't wait if request will block */ + __REQ_POST_READ_PROC, /* command specific flags for REQ_OP_WRITE_ZEROES: */ __REQ_NOUNMAP, /* do not free blocks when zeroing */ @@ -346,6 +347,7 @@ enum req_flag_bits { #define REQ_RAHEAD (1ULL << __REQ_RAHEAD) #define REQ_BACKGROUND (1ULL << __REQ_BACKGROUND) #define REQ_NOWAIT (1ULL << __REQ_NOWAIT) +#define REQ_POST_READ_PROC (1ULL << __REQ_POST_READ_PROC) #define REQ_NOUNMAP (1ULL << __REQ_NOUNMAP) #define REQ_HIPRI (1ULL << __REQ_HIPRI)
Ext4 and F2FS currently use a non-NULL value stored at bio->bi_private to determine if the contents of the bio need to be "post processed" i.e. whether its contents need to be decrypted and/or verified. For block size < page size scenario, bio->bi_private would hold a pointer to buffer_head. Hence, this commit adds the new flag REQ_POST_READ_PROC to be able to decisively check for post process requirement for a bio. Signed-off-by: Chandan Rajendra <chandan@linux.ibm.com> --- fs/ext4/readpage.c | 11 +++++++++-- fs/post_read_process.c | 2 +- include/linux/blk_types.h | 2 ++ 3 files changed, 12 insertions(+), 3 deletions(-)