From: Eric Biggers <ebiggers@kernel.org>
To: Chao Yu <chao@kernel.org>
Cc: jaegeuk@kernel.org, linux-f2fs-devel@lists.sourceforge.net,
linux-kernel@vger.kernel.org, Chao Yu <chao.yu@linux.dev>
Subject: Re: [f2fs-dev] [PATCH] f2fs: simplify accounting inflight directIO request
Date: Thu, 22 Jul 2021 08:48:15 -0700 [thread overview]
Message-ID: <YPmTP5EixgTp1Wze@gmail.com> (raw)
In-Reply-To: <20210722131617.749204-1-chao@kernel.org>
On Thu, Jul 22, 2021 at 09:16:17PM +0800, Chao Yu wrote:
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index ba120d55e9b1..d0a1ca6ae38e 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1720,6 +1720,9 @@ static int __get_data_block(struct inode *inode, sector_t iblock,
> map_bh(bh, inode->i_sb, map.m_pblk);
> bh->b_state = (bh->b_state & ~F2FS_MAP_FLAGS) | map.m_flags;
> bh->b_size = blks_to_bytes(inode, map.m_len);
> +
> + if (flag == F2FS_GET_BLOCK_DIO)
> + bh->b_private = (void *)may_write;
Why is this hunk needed?
> +static int f2fs_dio_end_io(struct kiocb *iocb, loff_t offset,
> + ssize_t bytes, void *private)
> {
> - 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);
> + struct inode *inode = file_inode(iocb->ki_filp);
> + bool may_write = private;
>
> - bio_endio(bio);
> + dec_dio_req_count(F2FS_I_SB(inode), may_write ? WRITE : READ);
> + return 0;
> }
>
> static void f2fs_dio_submit_bio(struct bio *bio, struct inode *inode,
> 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);
> + inc_dio_req_count(F2FS_I_SB(inode),
> + op_is_write(bio_op(bio)) ? WRITE : READ);
>
> submit_bio(bio);
> - return;
> -out:
> - bio->bi_status = BLK_STS_IOERR;
> - bio_endio(bio);
> }
The inc and dec here aren't correctly paired, since f2fs_dio_submit_bio() is
called once per bio whereas f2fs_dio_end_io() is called when the entire direct
I/O request (which may have consisted of multiple bios) has completed.
The correct way to do this would be to do the inc before calling
__blockdev_direct_IO(), and do the dec in end_io or if __blockdev_direct_IO()
returned without actually issuing any I/O.
But I think you shouldn't bother with this part of the change before we switch
to iomap, as it will then need to be changed again anyway.
- Eric
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
WARNING: multiple messages have this Message-ID (diff)
From: Eric Biggers <ebiggers@kernel.org>
To: Chao Yu <chao@kernel.org>
Cc: jaegeuk@kernel.org, Chao Yu <chao.yu@linux.dev>,
linux-kernel@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net
Subject: Re: [f2fs-dev] [PATCH] f2fs: simplify accounting inflight directIO request
Date: Thu, 22 Jul 2021 08:48:15 -0700 [thread overview]
Message-ID: <YPmTP5EixgTp1Wze@gmail.com> (raw)
In-Reply-To: <20210722131617.749204-1-chao@kernel.org>
On Thu, Jul 22, 2021 at 09:16:17PM +0800, Chao Yu wrote:
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index ba120d55e9b1..d0a1ca6ae38e 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1720,6 +1720,9 @@ static int __get_data_block(struct inode *inode, sector_t iblock,
> map_bh(bh, inode->i_sb, map.m_pblk);
> bh->b_state = (bh->b_state & ~F2FS_MAP_FLAGS) | map.m_flags;
> bh->b_size = blks_to_bytes(inode, map.m_len);
> +
> + if (flag == F2FS_GET_BLOCK_DIO)
> + bh->b_private = (void *)may_write;
Why is this hunk needed?
> +static int f2fs_dio_end_io(struct kiocb *iocb, loff_t offset,
> + ssize_t bytes, void *private)
> {
> - 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);
> + struct inode *inode = file_inode(iocb->ki_filp);
> + bool may_write = private;
>
> - bio_endio(bio);
> + dec_dio_req_count(F2FS_I_SB(inode), may_write ? WRITE : READ);
> + return 0;
> }
>
> static void f2fs_dio_submit_bio(struct bio *bio, struct inode *inode,
> 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);
> + inc_dio_req_count(F2FS_I_SB(inode),
> + op_is_write(bio_op(bio)) ? WRITE : READ);
>
> submit_bio(bio);
> - return;
> -out:
> - bio->bi_status = BLK_STS_IOERR;
> - bio_endio(bio);
> }
The inc and dec here aren't correctly paired, since f2fs_dio_submit_bio() is
called once per bio whereas f2fs_dio_end_io() is called when the entire direct
I/O request (which may have consisted of multiple bios) has completed.
The correct way to do this would be to do the inc before calling
__blockdev_direct_IO(), and do the dec in end_io or if __blockdev_direct_IO()
returned without actually issuing any I/O.
But I think you shouldn't bother with this part of the change before we switch
to iomap, as it will then need to be changed again anyway.
- Eric
next prev parent reply other threads:[~2021-07-22 15:48 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-22 13:16 [f2fs-dev] [PATCH] f2fs: simplify accounting inflight directIO request Chao Yu
2021-07-22 13:16 ` Chao Yu
2021-07-22 15:48 ` Eric Biggers [this message]
2021-07-22 15:48 ` [f2fs-dev] " Eric Biggers
2021-07-22 23:14 ` Chao Yu
2021-07-22 23:14 ` Chao Yu
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=YPmTP5EixgTp1Wze@gmail.com \
--to=ebiggers@kernel.org \
--cc=chao.yu@linux.dev \
--cc=chao@kernel.org \
--cc=jaegeuk@kernel.org \
--cc=linux-f2fs-devel@lists.sourceforge.net \
--cc=linux-kernel@vger.kernel.org \
/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: link
Be 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.