| Message ID | 20190816061804.14840-6-chandan@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Consolidate FS read I/O callbacks code | expand |
Hi Chandan, On 2019-8-16 14:18, Chandan Rajendra wrote: > F2FS has a copy of "post read processing" code using which encrypted > file data is decrypted. This commit replaces it to make use of the > generic read_callbacks facility. I remember that previously Jaegeuk had mentioned f2fs will support compression later, and it needs to reuse 'post read processing' fwk. There is very initial version of compression feature in below link: https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=compression So my concern is how can we uplift the most common parts of this fwk into vfs, and meanwhile keeping the ability and flexibility when introducing private feature/step in specified filesytem(now f2fs)? According to current f2fs compression's requirement, maybe we can expand to - support callback to let filesystem set the function for the flow in decompression/verity/decryption step. - support to use individual/common workqueue according the parameter. Any thoughts? Thanks, > > Signed-off-by: Chandan Rajendra <chandan@linux.ibm.com> > --- > fs/f2fs/data.c | 109 ++++-------------------------------------------- > fs/f2fs/f2fs.h | 2 - > fs/f2fs/super.c | 9 +--- > 3 files changed, 11 insertions(+), 109 deletions(-) > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index 757f050c650a..3cf1eca2ece9 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -18,6 +18,7 @@ > #include <linux/uio.h> > #include <linux/cleancache.h> > #include <linux/sched/signal.h> > +#include <linux/read_callbacks.h> > > #include "f2fs.h" > #include "node.h" > @@ -25,11 +26,6 @@ > #include "trace.h" > #include <trace/events/f2fs.h> > > -#define NUM_PREALLOC_POST_READ_CTXS 128 > - > -static struct kmem_cache *bio_post_read_ctx_cache; > -static mempool_t *bio_post_read_ctx_pool; > - > static bool __is_cp_guaranteed(struct page *page) > { > struct address_space *mapping = page->mapping; > @@ -69,19 +65,6 @@ static enum count_type __read_io_type(struct page *page) > return F2FS_RD_DATA; > } > > -/* postprocessing steps for read bios */ > -enum bio_post_read_step { > - STEP_INITIAL = 0, > - STEP_DECRYPT, > -}; > - > -struct bio_post_read_ctx { > - struct bio *bio; > - struct work_struct work; > - unsigned int cur_step; > - unsigned int enabled_steps; > -}; > - > static void __read_end_io(struct bio *bio) > { > struct page *page; > @@ -93,7 +76,7 @@ static void __read_end_io(struct bio *bio) > page = bv->bv_page; > > /* PG_error was set if any post_read step failed */ > - if (bio->bi_status || PageError(page)) { > + if (bio->bi_status || read_callbacks_failed(page)) { > ClearPageUptodate(page); > /* will re-read again later */ > ClearPageError(page); > @@ -103,42 +86,8 @@ static void __read_end_io(struct bio *bio) > dec_page_count(F2FS_P_SB(page), __read_io_type(page)); > unlock_page(page); > } > - if (bio->bi_private) > - mempool_free(bio->bi_private, bio_post_read_ctx_pool); > - bio_put(bio); > -} > > -static void bio_post_read_processing(struct bio_post_read_ctx *ctx); > - > -static void decrypt_work(struct work_struct *work) > -{ > - struct bio_post_read_ctx *ctx = > - container_of(work, struct bio_post_read_ctx, work); > - > - fscrypt_decrypt_bio(ctx->bio); > - > - bio_post_read_processing(ctx); > -} > - > -static void bio_post_read_processing(struct bio_post_read_ctx *ctx) > -{ > - switch (++ctx->cur_step) { > - case STEP_DECRYPT: > - if (ctx->enabled_steps & (1 << STEP_DECRYPT)) { > - INIT_WORK(&ctx->work, decrypt_work); > - fscrypt_enqueue_decrypt_work(&ctx->work); > - return; > - } > - ctx->cur_step++; > - /* fall-through */ > - default: > - __read_end_io(ctx->bio); > - } > -} > - > -static bool f2fs_bio_post_read_required(struct bio *bio) > -{ > - return bio->bi_private && !bio->bi_status; > + bio_put(bio); > } > > static void f2fs_read_end_io(struct bio *bio) > @@ -149,15 +98,7 @@ static void f2fs_read_end_io(struct bio *bio) > bio->bi_status = BLK_STS_IOERR; > } > > - if (f2fs_bio_post_read_required(bio)) { > - struct bio_post_read_ctx *ctx = bio->bi_private; > - > - ctx->cur_step = STEP_INITIAL; > - bio_post_read_processing(ctx); > - return; > - } > - > - __read_end_io(bio); > + read_callbacks_endio_bio(bio, __read_end_io); > } > > static void f2fs_write_end_io(struct bio *bio) > @@ -556,8 +497,7 @@ static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr, > { > struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > struct bio *bio; > - struct bio_post_read_ctx *ctx; > - unsigned int post_read_steps = 0; > + int err; > > if (!f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC)) > return ERR_PTR(-EFAULT); > @@ -569,17 +509,10 @@ static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr, > bio->bi_end_io = f2fs_read_end_io; > bio_set_op_attrs(bio, REQ_OP_READ, op_flag); > > - if (f2fs_encrypted_file(inode)) > - post_read_steps |= 1 << STEP_DECRYPT; > - if (post_read_steps) { > - ctx = mempool_alloc(bio_post_read_ctx_pool, GFP_NOFS); > - if (!ctx) { > - bio_put(bio); > - return ERR_PTR(-ENOMEM); > - } > - ctx->bio = bio; > - ctx->enabled_steps = post_read_steps; > - bio->bi_private = ctx; > + err = read_callbacks_setup_bio(inode, bio); > + if (err) { > + bio_put(bio); > + return ERR_PTR(err); > } > > return bio; > @@ -2860,27 +2793,3 @@ void f2fs_clear_page_cache_dirty_tag(struct page *page) > PAGECACHE_TAG_DIRTY); > xa_unlock_irqrestore(&mapping->i_pages, flags); > } > - > -int __init f2fs_init_post_read_processing(void) > -{ > - bio_post_read_ctx_cache = KMEM_CACHE(bio_post_read_ctx, 0); > - if (!bio_post_read_ctx_cache) > - goto fail; > - bio_post_read_ctx_pool = > - mempool_create_slab_pool(NUM_PREALLOC_POST_READ_CTXS, > - bio_post_read_ctx_cache); > - if (!bio_post_read_ctx_pool) > - goto fail_free_cache; > - return 0; > - > -fail_free_cache: > - kmem_cache_destroy(bio_post_read_ctx_cache); > -fail: > - return -ENOMEM; > -} > - > -void __exit f2fs_destroy_post_read_processing(void) > -{ > - mempool_destroy(bio_post_read_ctx_pool); > - kmem_cache_destroy(bio_post_read_ctx_cache); > -} > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 87f75ebd2fd6..cea79321b794 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -3125,8 +3125,6 @@ void f2fs_destroy_checkpoint_caches(void); > /* > * data.c > */ > -int f2fs_init_post_read_processing(void); > -void f2fs_destroy_post_read_processing(void); > void f2fs_submit_merged_write(struct f2fs_sb_info *sbi, enum page_type type); > void f2fs_submit_merged_write_cond(struct f2fs_sb_info *sbi, > struct inode *inode, struct page *page, > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > index 11b3a039a188..d7bbb4f1fdb3 100644 > --- a/fs/f2fs/super.c > +++ b/fs/f2fs/super.c > @@ -3597,15 +3597,11 @@ static int __init init_f2fs_fs(void) > err = register_filesystem(&f2fs_fs_type); > if (err) > goto free_shrinker; > + > f2fs_create_root_stats(); > - err = f2fs_init_post_read_processing(); > - if (err) > - goto free_root_stats; > + > return 0; > > -free_root_stats: > - f2fs_destroy_root_stats(); > - unregister_filesystem(&f2fs_fs_type); > free_shrinker: > unregister_shrinker(&f2fs_shrinker_info); > free_sysfs: > @@ -3626,7 +3622,6 @@ static int __init init_f2fs_fs(void) > > static void __exit exit_f2fs_fs(void) > { > - f2fs_destroy_post_read_processing(); > f2fs_destroy_root_stats(); > unregister_filesystem(&f2fs_fs_type); > unregister_shrinker(&f2fs_shrinker_info); >
On Sunday, August 18, 2019 7:15:42 PM IST Chao Yu wrote: > Hi Chandan, > > On 2019-8-16 14:18, Chandan Rajendra wrote: > > F2FS has a copy of "post read processing" code using which encrypted > > file data is decrypted. This commit replaces it to make use of the > > generic read_callbacks facility. > > I remember that previously Jaegeuk had mentioned f2fs will support compression > later, and it needs to reuse 'post read processing' fwk. > > There is very initial version of compression feature in below link: > > https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=compression > > So my concern is how can we uplift the most common parts of this fwk into vfs, > and meanwhile keeping the ability and flexibility when introducing private > feature/step in specified filesytem(now f2fs)? > > According to current f2fs compression's requirement, maybe we can expand to > > - support callback to let filesystem set the function for the flow in > decompression/verity/decryption step. > - support to use individual/common workqueue according the parameter. > > Any thoughts? > Hi, F2FS can be made to use fscrypt's queue for decryption and hence can reuse "read callbacks" code for decrypting data. For decompression, we could have a STEP_MISC where we invoke a FS provided callback function for FS specific post read processing? Something like the following can be implemented in read_callbacks(), case STEP_MISC: if (ctx->enabled_steps & (1 << STEP_MISC)) { /* ctx->fs_misc() must process bio in a workqueue and later invoke read_callbacks() with bio->bi_private's value as an argument. */ ctx->fs_misc(ctx->bio); return; } ctx->cur_step++; The fs_misc() callback can be passed in by the filesystem when invoking read_callbacks_setup_bio().
On Friday, August 16, 2019 11:48 AM Chandan Rajendra wrote: > F2FS has a copy of "post read processing" code using which encrypted > file data is decrypted. This commit replaces it to make use of the > generic read_callbacks facility. > > Signed-off-by: Chandan Rajendra <chandan@linux.ibm.com> Hi Eric and Ted, Looks like F2FS requires a lot more flexiblity than what can be offered by read callbacks i.e. 1. F2FS wants to make use of its own workqueue for decryption, verity and decompression. 2. F2FS' decompression code is not an FS independent entity like fscrypt and fsverity. Hence they would need Filesystem specific callback functions to be invoked from "read callbacks". Hence I would suggest that we should drop F2FS changes made in this patchset. Please let me know your thoughts on this.
Hi Chandan, On Tue, Aug 20, 2019 at 10:35:29AM +0530, Chandan Rajendra wrote: > On Friday, August 16, 2019 11:48 AM Chandan Rajendra wrote: > > F2FS has a copy of "post read processing" code using which encrypted > > file data is decrypted. This commit replaces it to make use of the > > generic read_callbacks facility. > > > > Signed-off-by: Chandan Rajendra <chandan@linux.ibm.com> > > Hi Eric and Ted, > > Looks like F2FS requires a lot more flexiblity than what can be offered by > read callbacks i.e. > > 1. F2FS wants to make use of its own workqueue for decryption, verity and > decompression. > 2. F2FS' decompression code is not an FS independent entity like fscrypt and > fsverity. Hence they would need Filesystem specific callback functions to > be invoked from "read callbacks". > > Hence I would suggest that we should drop F2FS changes made in this > patchset. Please let me know your thoughts on this. Add a word, I have some little concern about post read procession order a bit as I mentioned before, because I'd like to move common EROFS decompression code out in the future as well for other fses to use after we think it's mature enough. It seems the current code mainly addresses eliminating duplicated code, therefore I have no idea about that... Thanks, Gao Xiang > > -- > chandan > > >
On Tue, Aug 20, 2019 at 01:12:36PM +0800, Gao Xiang wrote: > Hi Chandan, > > On Tue, Aug 20, 2019 at 10:35:29AM +0530, Chandan Rajendra wrote: > > On Friday, August 16, 2019 11:48 AM Chandan Rajendra wrote: > > > F2FS has a copy of "post read processing" code using which encrypted > > > file data is decrypted. This commit replaces it to make use of the > > > generic read_callbacks facility. > > > > > > Signed-off-by: Chandan Rajendra <chandan@linux.ibm.com> > > > > Hi Eric and Ted, > > > > Looks like F2FS requires a lot more flexiblity than what can be offered by > > read callbacks i.e. > > > > 1. F2FS wants to make use of its own workqueue for decryption, verity and > > decompression. > > 2. F2FS' decompression code is not an FS independent entity like fscrypt and > > fsverity. Hence they would need Filesystem specific callback functions to > > be invoked from "read callbacks". > > > > Hence I would suggest that we should drop F2FS changes made in this > > patchset. Please let me know your thoughts on this. > > Add a word, I have some little concern about post read procession order FYI. Just a minor concern about its flexibility, not big though. https://lore.kernel.org/r/20190808042640.GA28630@138/ Thanks, Gao Xiang > a bit as I mentioned before, because I'd like to move common EROFS > decompression code out in the future as well for other fses to use > after we think it's mature enough. > > It seems the current code mainly addresses eliminating duplicated code, > therefore I have no idea about that... > > Thanks, > Gao Xiang > > > > > -- > > chandan > > > > > >
On 2019/8/19 21:33, Chandan Rajendra wrote: > On Sunday, August 18, 2019 7:15:42 PM IST Chao Yu wrote: >> Hi Chandan, >> >> On 2019-8-16 14:18, Chandan Rajendra wrote: >>> F2FS has a copy of "post read processing" code using which encrypted >>> file data is decrypted. This commit replaces it to make use of the >>> generic read_callbacks facility. >> >> I remember that previously Jaegeuk had mentioned f2fs will support compression >> later, and it needs to reuse 'post read processing' fwk. >> >> There is very initial version of compression feature in below link: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=compression >> >> So my concern is how can we uplift the most common parts of this fwk into vfs, >> and meanwhile keeping the ability and flexibility when introducing private >> feature/step in specified filesytem(now f2fs)? >> >> According to current f2fs compression's requirement, maybe we can expand to >> >> - support callback to let filesystem set the function for the flow in >> decompression/verity/decryption step. >> - support to use individual/common workqueue according the parameter. >> >> Any thoughts? >> > > Hi, > > F2FS can be made to use fscrypt's queue for decryption and hence can reuse > "read callbacks" code for decrypting data. > > For decompression, we could have a STEP_MISC where we invoke a FS provided > callback function for FS specific post read processing? > > Something like the following can be implemented in read_callbacks(), > case STEP_MISC: > if (ctx->enabled_steps & (1 << STEP_MISC)) { > /* > ctx->fs_misc() must process bio in a workqueue > and later invoke read_callbacks() with > bio->bi_private's value as an argument. > */ > ctx->fs_misc(ctx->bio); > return; > } > ctx->cur_step++; > > The fs_misc() callback can be passed in by the filesystem when invoking > read_callbacks_setup_bio(). Hi, Yes, something like this, can we just use STEP_DECOMPRESS and fs_decompress(), not sure, I doubt this interface may has potential user which has compression feature. One more concern is that to avoid more context switch, maybe we can merge all background works into one workqueue if there is no conflict when call wants to. static void bio_post_read_processing(struct bio_post_read_ctx *ctx) { switch (++ctx->cur_step) { case STEP_DECRYPT: if (ctx->enabled_steps & (1 << STEP_DECRYPT)) { ... if (ctx->separated_wq) return; } ctx->cur_step++; /* fall-through */ case STEP_DECOMPRESS: ... default: __read_end_io(ctx->bio); >
On Tue, Aug 20, 2019 at 01:12:36PM +0800, Gao Xiang wrote: > Add a word, I have some little concern about post read procession order > a bit as I mentioned before, because I'd like to move common EROFS > decompression code out in the future as well for other fses to use > after we think it's mature enough. > > It seems the current code mainly addresses eliminating duplicated code, > therefore I have no idea about that... Actually, we should chat. I was actually thinking about "borrowing" code from erofs to provide ext4-specific compression. I was really impressed with the efficiency goals in the erofs design[1] when I reviewed the Usenix ATC paper, and as the saying goes, the best artists know how to steal from the best. :-) [1] https://www.usenix.org/conference/atc19/presentation/gao My original specific thinking was to do code reuse by copy and paste, simply because it was simpler, and I have limited time to work on it. But if you are interested in making the erofs pipeline reusable by other file systems, and have the time to do the necessary code refactoring, I'd love to work with you on that. It should be noted that the f2fs developers have been working on their own compression scheme that was going to be f2fs-specific, unlike the file system generic approach used with fsverity and fscrypt. My expectation is that we will need to modify the read pipeling code to support compression. That's true whether we are looking at the existing file system-specific code used by ext4 and f2fs or in some combined work such as what Chandan has proposed. Cheers, - Ted
On Tue, Aug 20, 2019 at 10:35:29AM +0530, Chandan Rajendra wrote: > Looks like F2FS requires a lot more flexiblity than what can be offered by > read callbacks i.e. > > 1. F2FS wants to make use of its own workqueue for decryption, verity and > decompression. > 2. F2FS' decompression code is not an FS independent entity like fscrypt and > fsverity. Hence they would need Filesystem specific callback functions to > be invoked from "read callbacks". > > Hence I would suggest that we should drop F2FS changes made in this > patchset. Please let me know your thoughts on this. That's probably the best way to go for now. My one concern is that it means that only ext4 will be using your framework. I could imagine that some people might argue that should just move the callback scheme into ext4 code as opposed to leaving it in fscrypt --- at least until we can find other file systems where we can show that it will be useful for those other file systems. (Perhaps a useful experiment would be to have someone implement patches to support fscrypt and fsverity in ext2 --- the patch might or might not be accepted for upstream inclusion, but it would be useful to demonstrate how easy it is to add fscrypt and fsverity.) The other thing to consider is that there has been some discussion about adding generalized support for I/O submission to the iomap library. It might be that if that work is accepted, support for fscrypt and fsverity would be a requirement for ext4 to use that portion of iomap's functionality. So in that eventuality, it might be that we'll want to move your read callbacks code into iomap, or we'll need to rework the read callbacks code so it can work with iomap. But this is all work for the future. I'm a firm believe that the perfect should not be the enemy of the good, and that none of this should be a fundamental obstacle in having your code upstream. Cheers, - Ted
Hi Ted, On Tue, Aug 20, 2019 at 12:25:10PM -0400, Theodore Y. Ts'o wrote: > On Tue, Aug 20, 2019 at 01:12:36PM +0800, Gao Xiang wrote: > > Add a word, I have some little concern about post read procession order > > a bit as I mentioned before, because I'd like to move common EROFS > > decompression code out in the future as well for other fses to use > > after we think it's mature enough. > > > > It seems the current code mainly addresses eliminating duplicated code, > > therefore I have no idea about that... > > Actually, we should chat. I was actually thinking about "borrowing" > code from erofs to provide ext4-specific compression. I was really > impressed with the efficiency goals in the erofs design[1] when I > reviewed the Usenix ATC paper, and as the saying goes, the best > artists know how to steal from the best. :-) > > [1] https://www.usenix.org/conference/atc19/presentation/gao I also guessed it's you reviewed our work as well from some written words :) (even though it's analymous...) and I personally think there are some useful stuffs in our EROFS effort. > > My original specific thinking was to do code reuse by copy and paste, > simply because it was simpler, and I have limited time to work on it. > But if you are interested in making the erofs pipeline reusable by > other file systems, and have the time to do the necessary code > refactoring, I'd love to work with you on that. Yes, I have interest in making the erofs pipeline for generic fses. Now I'm still investigating sequential read on very high speed NVME (like SAMSUNG 970pro, one thread seq read >3GB/s), it seems it still has some optimization space. And then I will do that work for generic fses as well... (but the first thing I want to do is getting erofs out of staging, as Greg said [1]) Metadata should be designed for each fs like ext4, but maybe not flexible and compacted as EROFS, therefore it could be some extra metadata overhead than EROFS. [1] https://lore.kernel.org/lkml/20190618064523.GA6015@kroah.com/ > > It should be noted that the f2fs developers have been working on their > own compression scheme that was going to be f2fs-specific, unlike the > file system generic approach used with fsverity and fscrypt. > > My expectation is that we will need to modify the read pipeling code > to support compression. That's true whether we are looking at the > existing file system-specific code used by ext4 and f2fs or in some > combined work such as what Chandan has proposed. I think either form is fine with me. :) But it seems that is some minor which tree we will work on (Maybe Chandan's work will be merged then). The first thing I need to do is to tidy up the code, and making it more general, and then it will be very easy for fses to integrate :) Thanks, Gao Xiang > > Cheers, > > - Ted
Hi Chandan, On 08/20, Theodore Y. Ts'o wrote: > On Tue, Aug 20, 2019 at 10:35:29AM +0530, Chandan Rajendra wrote: > > Looks like F2FS requires a lot more flexiblity than what can be offered by > > read callbacks i.e. > > > > 1. F2FS wants to make use of its own workqueue for decryption, verity and > > decompression. > > 2. F2FS' decompression code is not an FS independent entity like fscrypt and > > fsverity. Hence they would need Filesystem specific callback functions to > > be invoked from "read callbacks". > > > > Hence I would suggest that we should drop F2FS changes made in this > > patchset. Please let me know your thoughts on this. > > That's probably the best way to go for now. My one concern is that it > means that only ext4 will be using your framework. I could imagine > that some people might argue that should just move the callback scheme > into ext4 code as opposed to leaving it in fscrypt --- at least until > we can find other file systems where we can show that it will be > useful for those other file systems. I also have to raise a flag on this. Doesn't this patch series try to get rid of redundant work? What'd be the rationale, if it only supports ext4? How about generalizing the framework to support generic_post_read and per-fs post_read for fscrypt/fsverity/... selectively? Thanks, > > (Perhaps a useful experiment would be to have someone implement patches > to support fscrypt and fsverity in ext2 --- the patch might or might > not be accepted for upstream inclusion, but it would be useful to > demonstrate how easy it is to add fscrypt and fsverity.) > > The other thing to consider is that there has been some discussion > about adding generalized support for I/O submission to the iomap > library. It might be that if that work is accepted, support for > fscrypt and fsverity would be a requirement for ext4 to use that > portion of iomap's functionality. So in that eventuality, it might be > that we'll want to move your read callbacks code into iomap, or we'll > need to rework the read callbacks code so it can work with iomap. > > But this is all work for the future. I'm a firm believe that the > perfect should not be the enemy of the good, and that none of this > should be a fundamental obstacle in having your code upstream. > > Cheers, > > - Ted >
On Tuesday, August 20, 2019 11:01 PM Jaegeuk Kim wrote: > Hi Chandan, > > On 08/20, Theodore Y. Ts'o wrote: > > On Tue, Aug 20, 2019 at 10:35:29AM +0530, Chandan Rajendra wrote: > > > Looks like F2FS requires a lot more flexiblity than what can be offered by > > > read callbacks i.e. > > > > > > 1. F2FS wants to make use of its own workqueue for decryption, verity and > > > decompression. > > > 2. F2FS' decompression code is not an FS independent entity like fscrypt and > > > fsverity. Hence they would need Filesystem specific callback functions to > > > be invoked from "read callbacks". > > > > > > Hence I would suggest that we should drop F2FS changes made in this > > > patchset. Please let me know your thoughts on this. > > > > That's probably the best way to go for now. My one concern is that it > > means that only ext4 will be using your framework. I could imagine > > that some people might argue that should just move the callback scheme > > into ext4 code as opposed to leaving it in fscrypt --- at least until > > we can find other file systems where we can show that it will be > > useful for those other file systems. > > I also have to raise a flag on this. Doesn't this patch series try to get rid > of redundant work? What'd be the rationale, if it only supports ext4? This patchset gets encryption working with subpage blocksize by making relevant changes in the generic code (i.e. do_mpage_readpage() and block_read_full_page()) and removing duplicate code from ext4 (i.e. ext4_readpage() and friends). Without these changes the only way to get subpage blocksize support was to add more duplicate code into Ext4 i.e. import a copy of block_read_full_page() into Ext4 and make necessary edits to support encryption. So this patchset actually does help in removing exiting duplicate code in Ext4 and also prevents addition of more such code. > > How about generalizing the framework to support generic_post_read and per-fs > post_read for fscrypt/fsverity/... selectively? Quoting what I had said earlier, > > > 1. F2FS wants to make use of its own workqueue for decryption, verity and > > > decompression. > > > 2. F2FS' decompression code is not an FS independent entity like fscrypt and > > > fsverity. Hence they would need Filesystem specific callback functions to > > > be invoked from "read callbacks". I am not sure if read callbacks can be made flexible enough to support the above use cases. fscrypt and fsverity already provide workqueues and any new post processing code added should follow the same convention. I see that F2FS use case is special since, 1. It uses its own workqueues. 2. Decompression code inside F2FS isn't written as an FS independent subsystem like how fscrypt and fsverity are implemented. To summarize, I believe the users of read callbacks should follow the conventions set by fscrypt/fsverity and new post processing code that needs to be plugged into read callbacks should provide APIs similar to fscrypt/fsverity. Otherwise the state machine logic implemented by read callbacks will get complex/convoluted. > > Thanks, > > > > > (Perhaps a useful experiment would be to have someone implement patches > > to support fscrypt and fsverity in ext2 --- the patch might or might > > not be accepted for upstream inclusion, but it would be useful to > > demonstrate how easy it is to add fscrypt and fsverity.) > > > > The other thing to consider is that there has been some discussion > > about adding generalized support for I/O submission to the iomap > > library. It might be that if that work is accepted, support for > > fscrypt and fsverity would be a requirement for ext4 to use that > > portion of iomap's functionality. So in that eventuality, it might be > > that we'll want to move your read callbacks code into iomap, or we'll > > need to rework the read callbacks code so it can work with iomap. > > > > But this is all work for the future. I'm a firm believe that the > > perfect should not be the enemy of the good, and that none of this > > should be a fundamental obstacle in having your code upstream. > > > > Cheers, > > > > - Ted > > >
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index 757f050c650a..3cf1eca2ece9 100644 --- a/fs/f2fs/data.c +++ b/fs/f2fs/data.c @@ -18,6 +18,7 @@ #include <linux/uio.h> #include <linux/cleancache.h> #include <linux/sched/signal.h> +#include <linux/read_callbacks.h> #include "f2fs.h" #include "node.h" @@ -25,11 +26,6 @@ #include "trace.h" #include <trace/events/f2fs.h> -#define NUM_PREALLOC_POST_READ_CTXS 128 - -static struct kmem_cache *bio_post_read_ctx_cache; -static mempool_t *bio_post_read_ctx_pool; - static bool __is_cp_guaranteed(struct page *page) { struct address_space *mapping = page->mapping; @@ -69,19 +65,6 @@ static enum count_type __read_io_type(struct page *page) return F2FS_RD_DATA; } -/* postprocessing steps for read bios */ -enum bio_post_read_step { - STEP_INITIAL = 0, - STEP_DECRYPT, -}; - -struct bio_post_read_ctx { - struct bio *bio; - struct work_struct work; - unsigned int cur_step; - unsigned int enabled_steps; -}; - static void __read_end_io(struct bio *bio) { struct page *page; @@ -93,7 +76,7 @@ static void __read_end_io(struct bio *bio) page = bv->bv_page; /* PG_error was set if any post_read step failed */ - if (bio->bi_status || PageError(page)) { + if (bio->bi_status || read_callbacks_failed(page)) { ClearPageUptodate(page); /* will re-read again later */ ClearPageError(page); @@ -103,42 +86,8 @@ static void __read_end_io(struct bio *bio) dec_page_count(F2FS_P_SB(page), __read_io_type(page)); unlock_page(page); } - if (bio->bi_private) - mempool_free(bio->bi_private, bio_post_read_ctx_pool); - bio_put(bio); -} -static void bio_post_read_processing(struct bio_post_read_ctx *ctx); - -static void decrypt_work(struct work_struct *work) -{ - struct bio_post_read_ctx *ctx = - container_of(work, struct bio_post_read_ctx, work); - - fscrypt_decrypt_bio(ctx->bio); - - bio_post_read_processing(ctx); -} - -static void bio_post_read_processing(struct bio_post_read_ctx *ctx) -{ - switch (++ctx->cur_step) { - case STEP_DECRYPT: - if (ctx->enabled_steps & (1 << STEP_DECRYPT)) { - INIT_WORK(&ctx->work, decrypt_work); - fscrypt_enqueue_decrypt_work(&ctx->work); - return; - } - ctx->cur_step++; - /* fall-through */ - default: - __read_end_io(ctx->bio); - } -} - -static bool f2fs_bio_post_read_required(struct bio *bio) -{ - return bio->bi_private && !bio->bi_status; + bio_put(bio); } static void f2fs_read_end_io(struct bio *bio) @@ -149,15 +98,7 @@ static void f2fs_read_end_io(struct bio *bio) bio->bi_status = BLK_STS_IOERR; } - if (f2fs_bio_post_read_required(bio)) { - struct bio_post_read_ctx *ctx = bio->bi_private; - - ctx->cur_step = STEP_INITIAL; - bio_post_read_processing(ctx); - return; - } - - __read_end_io(bio); + read_callbacks_endio_bio(bio, __read_end_io); } static void f2fs_write_end_io(struct bio *bio) @@ -556,8 +497,7 @@ static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr, { struct f2fs_sb_info *sbi = F2FS_I_SB(inode); struct bio *bio; - struct bio_post_read_ctx *ctx; - unsigned int post_read_steps = 0; + int err; if (!f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC)) return ERR_PTR(-EFAULT); @@ -569,17 +509,10 @@ static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr, bio->bi_end_io = f2fs_read_end_io; bio_set_op_attrs(bio, REQ_OP_READ, op_flag); - if (f2fs_encrypted_file(inode)) - post_read_steps |= 1 << STEP_DECRYPT; - if (post_read_steps) { - ctx = mempool_alloc(bio_post_read_ctx_pool, GFP_NOFS); - if (!ctx) { - bio_put(bio); - return ERR_PTR(-ENOMEM); - } - ctx->bio = bio; - ctx->enabled_steps = post_read_steps; - bio->bi_private = ctx; + err = read_callbacks_setup_bio(inode, bio); + if (err) { + bio_put(bio); + return ERR_PTR(err); } return bio; @@ -2860,27 +2793,3 @@ void f2fs_clear_page_cache_dirty_tag(struct page *page) PAGECACHE_TAG_DIRTY); xa_unlock_irqrestore(&mapping->i_pages, flags); } - -int __init f2fs_init_post_read_processing(void) -{ - bio_post_read_ctx_cache = KMEM_CACHE(bio_post_read_ctx, 0); - if (!bio_post_read_ctx_cache) - goto fail; - bio_post_read_ctx_pool = - mempool_create_slab_pool(NUM_PREALLOC_POST_READ_CTXS, - bio_post_read_ctx_cache); - if (!bio_post_read_ctx_pool) - goto fail_free_cache; - return 0; - -fail_free_cache: - kmem_cache_destroy(bio_post_read_ctx_cache); -fail: - return -ENOMEM; -} - -void __exit f2fs_destroy_post_read_processing(void) -{ - mempool_destroy(bio_post_read_ctx_pool); - kmem_cache_destroy(bio_post_read_ctx_cache); -} diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 87f75ebd2fd6..cea79321b794 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -3125,8 +3125,6 @@ void f2fs_destroy_checkpoint_caches(void); /* * data.c */ -int f2fs_init_post_read_processing(void); -void f2fs_destroy_post_read_processing(void); void f2fs_submit_merged_write(struct f2fs_sb_info *sbi, enum page_type type); void f2fs_submit_merged_write_cond(struct f2fs_sb_info *sbi, struct inode *inode, struct page *page, diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c index 11b3a039a188..d7bbb4f1fdb3 100644 --- a/fs/f2fs/super.c +++ b/fs/f2fs/super.c @@ -3597,15 +3597,11 @@ static int __init init_f2fs_fs(void) err = register_filesystem(&f2fs_fs_type); if (err) goto free_shrinker; + f2fs_create_root_stats(); - err = f2fs_init_post_read_processing(); - if (err) - goto free_root_stats; + return 0; -free_root_stats: - f2fs_destroy_root_stats(); - unregister_filesystem(&f2fs_fs_type); free_shrinker: unregister_shrinker(&f2fs_shrinker_info); free_sysfs: @@ -3626,7 +3622,6 @@ static int __init init_f2fs_fs(void) static void __exit exit_f2fs_fs(void) { - f2fs_destroy_post_read_processing(); f2fs_destroy_root_stats(); unregister_filesystem(&f2fs_fs_type); unregister_shrinker(&f2fs_shrinker_info);
F2FS has a copy of "post read processing" code using which encrypted file data is decrypted. This commit replaces it to make use of the generic read_callbacks facility. Signed-off-by: Chandan Rajendra <chandan@linux.ibm.com> --- fs/f2fs/data.c | 109 ++++-------------------------------------------- fs/f2fs/f2fs.h | 2 - fs/f2fs/super.c | 9 +--- 3 files changed, 11 insertions(+), 109 deletions(-)