From: Jaegeuk Kim <jaegeuk@kernel.org> To: Christoph Hellwig <hch@infradead.org> Cc: Eric Biggers <ebiggers@kernel.org>, linux-f2fs-devel@lists.sourceforge.net, Chao Yu <chao@kernel.org>, linux-fsdevel@vger.kernel.org, linux-xfs@vger.kernel.org, Satya Tangirala <satyaprateek2357@gmail.com>, Changheun Lee <nanich.lee@samsung.com>, Matthew Bobrowski <mbobrowski@mbobrowski.org> Subject: Re: [PATCH 6/9] f2fs: implement iomap operations Date: Thu, 22 Jul 2021 13:47:39 -0700 [thread overview] Message-ID: <YPnZa5dFVP7vtB9q@google.com> (raw) In-Reply-To: <YPU+3inGclUtcSpJ@infradead.org> On 07/19, Christoph Hellwig wrote: > On Fri, Jul 16, 2021 at 09:39:16AM -0500, Eric Biggers wrote: > > +static blk_qc_t f2fs_dio_submit_bio(struct inode *inode, struct iomap *iomap, > > + struct bio *bio, loff_t file_offset) > > +{ > > + struct f2fs_private_dio *dio; > > + bool write = (bio_op(bio) == REQ_OP_WRITE); > > + > > + dio = f2fs_kzalloc(F2FS_I_SB(inode), > > + sizeof(struct f2fs_private_dio), GFP_NOFS); > > + if (!dio) > > + goto out; > > + > > + dio->inode = inode; > > + dio->orig_end_io = bio->bi_end_io; > > + dio->orig_private = bio->bi_private; > > + dio->write = write; > > + > > + bio->bi_end_io = f2fs_dio_end_io; > > + bio->bi_private = dio; > > + > > + inc_page_count(F2FS_I_SB(inode), > > + write ? F2FS_DIO_WRITE : F2FS_DIO_READ); > > + > > + return submit_bio(bio); > > I don't think there is any need for this mess. The F2FS_DIO_WRITE / > F2FS_DIO_READ counts are only used to check if there is any inflight > I/O at all. So instead we can increment them once before calling > iomap_dio_rw, and decrement them in ->end_io or for a failure/noop > exit from iomap_dio_rw. Untested patch below. Note that all this > would be much simpler to review if the last three patches were folded > into a single one. Eric, wdyt? I've merged v1 to v5, including Christoph's comment in v2. > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index 4fbf28f5aaab..9f9cc49fbe94 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -3369,50 +3369,6 @@ static int f2fs_write_end(struct file *file, > return copied; > } > > -static void f2fs_dio_end_io(struct bio *bio) > -{ > - struct f2fs_private_dio *dio = bio->bi_private; > - > - dec_page_count(F2FS_I_SB(dio->inode), > - dio->write ? F2FS_DIO_WRITE : F2FS_DIO_READ); > - > - bio->bi_private = dio->orig_private; > - bio->bi_end_io = dio->orig_end_io; > - > - kfree(dio); > - > - bio_endio(bio); > -} > - > -static blk_qc_t f2fs_dio_submit_bio(struct inode *inode, struct iomap *iomap, > - struct bio *bio, loff_t file_offset) > -{ > - struct f2fs_private_dio *dio; > - bool write = (bio_op(bio) == REQ_OP_WRITE); > - > - dio = f2fs_kzalloc(F2FS_I_SB(inode), > - sizeof(struct f2fs_private_dio), GFP_NOFS); > - if (!dio) > - goto out; > - > - dio->inode = inode; > - dio->orig_end_io = bio->bi_end_io; > - dio->orig_private = bio->bi_private; > - dio->write = write; > - > - bio->bi_end_io = f2fs_dio_end_io; > - bio->bi_private = dio; > - > - inc_page_count(F2FS_I_SB(inode), > - write ? F2FS_DIO_WRITE : F2FS_DIO_READ); > - > - return submit_bio(bio); > -out: > - bio->bi_status = BLK_STS_IOERR; > - bio_endio(bio); > - return BLK_QC_T_NONE; > -} > - > void f2fs_invalidate_page(struct page *page, unsigned int offset, > unsigned int length) > { > @@ -4006,6 +3962,18 @@ const struct iomap_ops f2fs_iomap_ops = { > .iomap_begin = f2fs_iomap_begin, > }; > > +static int f2fs_dio_end_io(struct kiocb *iocb, ssize_t size, int error, > + unsigned flags) > +{ > + struct f2fs_sb_info *sbi = F2FS_I_SB(file_inode(iocb->ki_filp)); > + > + if (iocb->ki_flags & IOCB_WRITE) > + dec_page_count(sbi, F2FS_DIO_WRITE); > + else > + dec_page_count(sbi, F2FS_DIO_READ); > + return 0; > +} > + > const struct iomap_dio_ops f2fs_iomap_dio_ops = { > - .submit_io = f2fs_dio_submit_bio, > + .end_io = f2fs_dio_end_io, > }; > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 6dbbac05a15c..abd521dc504a 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -1750,13 +1750,6 @@ struct f2fs_sb_info { > #endif > }; > > -struct f2fs_private_dio { > - struct inode *inode; > - void *orig_private; > - bio_end_io_t *orig_end_io; > - bool write; > -}; > - > #ifdef CONFIG_F2FS_FAULT_INJECTION > #define f2fs_show_injection_info(sbi, type) \ > printk_ratelimited("%sF2FS-fs (%s) : inject %s in %s of %pS\n", \ > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > index 6b8eac6b25d4..4fed90cc1462 100644 > --- a/fs/f2fs/file.c > +++ b/fs/f2fs/file.c > @@ -4259,6 +4259,7 @@ static ssize_t f2fs_dio_read_iter(struct kiocb *iocb, struct iov_iter *to) > down_read(&fi->i_gc_rwsem[READ]); > } > > + inc_page_count(F2FS_I_SB(inode), F2FS_DIO_READ); > ret = iomap_dio_rw(iocb, to, &f2fs_iomap_ops, &f2fs_iomap_dio_ops, 0); > > up_read(&fi->i_gc_rwsem[READ]); > @@ -4270,6 +4271,8 @@ static ssize_t f2fs_dio_read_iter(struct kiocb *iocb, struct iov_iter *to) > else if (ret == -EIOCBQUEUED) > f2fs_update_iostat(F2FS_I_SB(inode), APP_DIRECT_READ_IO, > count - iov_iter_count(to)); > + else > + dec_page_count(F2FS_I_SB(inode), F2FS_DIO_READ); > out: > trace_f2fs_direct_IO_exit(inode, pos, count, READ, ret); > return ret; > @@ -4446,6 +4449,7 @@ static ssize_t f2fs_dio_write_iter(struct kiocb *iocb, struct iov_iter *from, > > if (pos + count > inode->i_size) > dio_flags |= IOMAP_DIO_FORCE_WAIT; > + inc_page_count(F2FS_I_SB(inode), F2FS_DIO_WRITE); > ret = iomap_dio_rw(iocb, from, &f2fs_iomap_ops, &f2fs_iomap_dio_ops, > dio_flags); > if (ret == -ENOTBLK) > @@ -4459,6 +4463,9 @@ static ssize_t f2fs_dio_write_iter(struct kiocb *iocb, struct iov_iter *from, > > up_read(&fi->i_gc_rwsem[WRITE]); > > + if (ret <= 0 && ret != -EIOCBQUEUED) > + dec_page_count(F2FS_I_SB(inode), F2FS_DIO_WRITE); > + > if (ret < 0) { > if (ret == -EIOCBQUEUED) > f2fs_update_iostat(sbi, APP_DIRECT_IO,
WARNING: multiple messages have this Message-ID (diff)
From: Jaegeuk Kim <jaegeuk@kernel.org> To: Christoph Hellwig <hch@infradead.org> Cc: Satya Tangirala <satyaprateek2357@gmail.com>, linux-xfs@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, Eric Biggers <ebiggers@kernel.org>, Matthew Bobrowski <mbobrowski@mbobrowski.org>, Changheun Lee <nanich.lee@samsung.com>, linux-fsdevel@vger.kernel.org Subject: Re: [f2fs-dev] [PATCH 6/9] f2fs: implement iomap operations Date: Thu, 22 Jul 2021 13:47:39 -0700 [thread overview] Message-ID: <YPnZa5dFVP7vtB9q@google.com> (raw) In-Reply-To: <YPU+3inGclUtcSpJ@infradead.org> On 07/19, Christoph Hellwig wrote: > On Fri, Jul 16, 2021 at 09:39:16AM -0500, Eric Biggers wrote: > > +static blk_qc_t f2fs_dio_submit_bio(struct inode *inode, struct iomap *iomap, > > + struct bio *bio, loff_t file_offset) > > +{ > > + struct f2fs_private_dio *dio; > > + bool write = (bio_op(bio) == REQ_OP_WRITE); > > + > > + dio = f2fs_kzalloc(F2FS_I_SB(inode), > > + sizeof(struct f2fs_private_dio), GFP_NOFS); > > + if (!dio) > > + goto out; > > + > > + dio->inode = inode; > > + dio->orig_end_io = bio->bi_end_io; > > + dio->orig_private = bio->bi_private; > > + dio->write = write; > > + > > + bio->bi_end_io = f2fs_dio_end_io; > > + bio->bi_private = dio; > > + > > + inc_page_count(F2FS_I_SB(inode), > > + write ? F2FS_DIO_WRITE : F2FS_DIO_READ); > > + > > + return submit_bio(bio); > > I don't think there is any need for this mess. The F2FS_DIO_WRITE / > F2FS_DIO_READ counts are only used to check if there is any inflight > I/O at all. So instead we can increment them once before calling > iomap_dio_rw, and decrement them in ->end_io or for a failure/noop > exit from iomap_dio_rw. Untested patch below. Note that all this > would be much simpler to review if the last three patches were folded > into a single one. Eric, wdyt? I've merged v1 to v5, including Christoph's comment in v2. > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index 4fbf28f5aaab..9f9cc49fbe94 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -3369,50 +3369,6 @@ static int f2fs_write_end(struct file *file, > return copied; > } > > -static void f2fs_dio_end_io(struct bio *bio) > -{ > - struct f2fs_private_dio *dio = bio->bi_private; > - > - dec_page_count(F2FS_I_SB(dio->inode), > - dio->write ? F2FS_DIO_WRITE : F2FS_DIO_READ); > - > - bio->bi_private = dio->orig_private; > - bio->bi_end_io = dio->orig_end_io; > - > - kfree(dio); > - > - bio_endio(bio); > -} > - > -static blk_qc_t f2fs_dio_submit_bio(struct inode *inode, struct iomap *iomap, > - struct bio *bio, loff_t file_offset) > -{ > - struct f2fs_private_dio *dio; > - bool write = (bio_op(bio) == REQ_OP_WRITE); > - > - dio = f2fs_kzalloc(F2FS_I_SB(inode), > - sizeof(struct f2fs_private_dio), GFP_NOFS); > - if (!dio) > - goto out; > - > - dio->inode = inode; > - dio->orig_end_io = bio->bi_end_io; > - dio->orig_private = bio->bi_private; > - dio->write = write; > - > - bio->bi_end_io = f2fs_dio_end_io; > - bio->bi_private = dio; > - > - inc_page_count(F2FS_I_SB(inode), > - write ? F2FS_DIO_WRITE : F2FS_DIO_READ); > - > - return submit_bio(bio); > -out: > - bio->bi_status = BLK_STS_IOERR; > - bio_endio(bio); > - return BLK_QC_T_NONE; > -} > - > void f2fs_invalidate_page(struct page *page, unsigned int offset, > unsigned int length) > { > @@ -4006,6 +3962,18 @@ const struct iomap_ops f2fs_iomap_ops = { > .iomap_begin = f2fs_iomap_begin, > }; > > +static int f2fs_dio_end_io(struct kiocb *iocb, ssize_t size, int error, > + unsigned flags) > +{ > + struct f2fs_sb_info *sbi = F2FS_I_SB(file_inode(iocb->ki_filp)); > + > + if (iocb->ki_flags & IOCB_WRITE) > + dec_page_count(sbi, F2FS_DIO_WRITE); > + else > + dec_page_count(sbi, F2FS_DIO_READ); > + return 0; > +} > + > const struct iomap_dio_ops f2fs_iomap_dio_ops = { > - .submit_io = f2fs_dio_submit_bio, > + .end_io = f2fs_dio_end_io, > }; > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 6dbbac05a15c..abd521dc504a 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -1750,13 +1750,6 @@ struct f2fs_sb_info { > #endif > }; > > -struct f2fs_private_dio { > - struct inode *inode; > - void *orig_private; > - bio_end_io_t *orig_end_io; > - bool write; > -}; > - > #ifdef CONFIG_F2FS_FAULT_INJECTION > #define f2fs_show_injection_info(sbi, type) \ > printk_ratelimited("%sF2FS-fs (%s) : inject %s in %s of %pS\n", \ > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > index 6b8eac6b25d4..4fed90cc1462 100644 > --- a/fs/f2fs/file.c > +++ b/fs/f2fs/file.c > @@ -4259,6 +4259,7 @@ static ssize_t f2fs_dio_read_iter(struct kiocb *iocb, struct iov_iter *to) > down_read(&fi->i_gc_rwsem[READ]); > } > > + inc_page_count(F2FS_I_SB(inode), F2FS_DIO_READ); > ret = iomap_dio_rw(iocb, to, &f2fs_iomap_ops, &f2fs_iomap_dio_ops, 0); > > up_read(&fi->i_gc_rwsem[READ]); > @@ -4270,6 +4271,8 @@ static ssize_t f2fs_dio_read_iter(struct kiocb *iocb, struct iov_iter *to) > else if (ret == -EIOCBQUEUED) > f2fs_update_iostat(F2FS_I_SB(inode), APP_DIRECT_READ_IO, > count - iov_iter_count(to)); > + else > + dec_page_count(F2FS_I_SB(inode), F2FS_DIO_READ); > out: > trace_f2fs_direct_IO_exit(inode, pos, count, READ, ret); > return ret; > @@ -4446,6 +4449,7 @@ static ssize_t f2fs_dio_write_iter(struct kiocb *iocb, struct iov_iter *from, > > if (pos + count > inode->i_size) > dio_flags |= IOMAP_DIO_FORCE_WAIT; > + inc_page_count(F2FS_I_SB(inode), F2FS_DIO_WRITE); > ret = iomap_dio_rw(iocb, from, &f2fs_iomap_ops, &f2fs_iomap_dio_ops, > dio_flags); > if (ret == -ENOTBLK) > @@ -4459,6 +4463,9 @@ static ssize_t f2fs_dio_write_iter(struct kiocb *iocb, struct iov_iter *from, > > up_read(&fi->i_gc_rwsem[WRITE]); > > + if (ret <= 0 && ret != -EIOCBQUEUED) > + dec_page_count(F2FS_I_SB(inode), F2FS_DIO_WRITE); > + > if (ret < 0) { > if (ret == -EIOCBQUEUED) > f2fs_update_iostat(sbi, APP_DIRECT_IO, _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
next prev parent reply other threads:[~2021-07-22 20:47 UTC|newest] Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-07-16 14:39 [PATCH 0/9] f2fs: use iomap for direct I/O Eric Biggers 2021-07-16 14:39 ` [f2fs-dev] " Eric Biggers 2021-07-16 14:39 ` [PATCH 1/9] f2fs: make f2fs_write_failed() take struct inode Eric Biggers 2021-07-16 14:39 ` [f2fs-dev] " Eric Biggers 2021-07-25 10:00 ` Chao Yu 2021-07-25 10:00 ` [f2fs-dev] " Chao Yu 2021-07-16 14:39 ` [PATCH 2/9] f2fs: remove allow_outplace_dio() Eric Biggers 2021-07-16 14:39 ` [f2fs-dev] " Eric Biggers 2021-07-19 8:41 ` Christoph Hellwig 2021-07-19 8:41 ` [f2fs-dev] " Christoph Hellwig 2021-07-16 14:39 ` [PATCH 3/9] f2fs: rework write preallocations Eric Biggers 2021-07-16 14:39 ` [f2fs-dev] " Eric Biggers 2021-07-25 10:50 ` Chao Yu 2021-07-25 10:50 ` [f2fs-dev] " Chao Yu 2021-07-25 17:57 ` Eric Biggers 2021-07-25 17:57 ` [f2fs-dev] " Eric Biggers 2021-07-27 2:00 ` Jaegeuk Kim 2021-07-27 2:00 ` [f2fs-dev] " Jaegeuk Kim 2021-07-27 3:23 ` Chao Yu 2021-07-27 3:23 ` [f2fs-dev] " Chao Yu 2021-07-27 7:38 ` Eric Biggers 2021-07-27 7:38 ` [f2fs-dev] " Eric Biggers 2021-07-27 8:30 ` Chao Yu 2021-07-27 8:30 ` [f2fs-dev] " Chao Yu 2021-07-27 15:33 ` Darrick J. Wong 2021-07-27 15:33 ` [f2fs-dev] " Darrick J. Wong 2021-07-29 0:26 ` Chao Yu 2021-07-29 0:26 ` [f2fs-dev] " Chao Yu 2021-07-28 2:29 ` Jaegeuk Kim 2021-07-28 2:29 ` [f2fs-dev] " Jaegeuk Kim 2021-07-25 15:35 ` Jaegeuk Kim 2021-07-25 15:35 ` [f2fs-dev] " Jaegeuk Kim 2021-07-25 15:47 ` Jaegeuk Kim 2021-07-25 15:47 ` [f2fs-dev] " Jaegeuk Kim 2021-07-25 18:01 ` Eric Biggers 2021-07-25 18:01 ` [f2fs-dev] " Eric Biggers 2021-07-26 19:04 ` Jaegeuk Kim 2021-07-26 19:04 ` [f2fs-dev] " Jaegeuk Kim 2021-07-16 14:39 ` [PATCH 4/9] f2fs: reduce indentation in f2fs_file_write_iter() Eric Biggers 2021-07-16 14:39 ` [f2fs-dev] " Eric Biggers 2021-07-16 14:39 ` [PATCH 5/9] f2fs: fix the f2fs_file_write_iter tracepoint Eric Biggers 2021-07-16 14:39 ` [f2fs-dev] " Eric Biggers 2021-07-16 14:39 ` [PATCH 6/9] f2fs: implement iomap operations Eric Biggers 2021-07-16 14:39 ` [f2fs-dev] " Eric Biggers 2021-07-19 8:59 ` Christoph Hellwig 2021-07-19 8:59 ` [f2fs-dev] " Christoph Hellwig 2021-07-22 20:47 ` Jaegeuk Kim [this message] 2021-07-22 20:47 ` Jaegeuk Kim 2021-07-22 20:49 ` Jaegeuk Kim 2021-07-22 20:49 ` [f2fs-dev] " Jaegeuk Kim 2021-07-22 20:54 ` Eric Biggers 2021-07-22 20:54 ` [f2fs-dev] " Eric Biggers 2021-07-22 21:57 ` Jaegeuk Kim 2021-07-22 21:57 ` [f2fs-dev] " Jaegeuk Kim 2021-07-23 1:52 ` Eric Biggers 2021-07-23 1:52 ` [f2fs-dev] " Eric Biggers 2021-07-23 5:00 ` Christoph Hellwig 2021-07-23 5:00 ` [f2fs-dev] " Christoph Hellwig 2021-07-23 8:05 ` Eric Biggers 2021-07-23 8:05 ` [f2fs-dev] " Eric Biggers 2021-07-16 14:39 ` [PATCH 7/9] f2fs: use iomap for direct I/O reads Eric Biggers 2021-07-16 14:39 ` [f2fs-dev] " Eric Biggers 2021-07-16 14:39 ` [PATCH 8/9] f2fs: use iomap for direct I/O writes Eric Biggers 2021-07-16 14:39 ` [f2fs-dev] " Eric Biggers 2021-07-16 14:39 ` [PATCH 9/9] f2fs: remove f2fs_direct_IO() Eric Biggers 2021-07-16 14:39 ` [f2fs-dev] " Eric Biggers
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YPnZa5dFVP7vtB9q@google.com \ --to=jaegeuk@kernel.org \ --cc=chao@kernel.org \ --cc=ebiggers@kernel.org \ --cc=hch@infradead.org \ --cc=linux-f2fs-devel@lists.sourceforge.net \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-xfs@vger.kernel.org \ --cc=mbobrowski@mbobrowski.org \ --cc=nanich.lee@samsung.com \ --cc=satyaprateek2357@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.