linux-f2fs-devel.lists.sourceforge.net archive mirror
 help / color / mirror / Atom feed
From: "常凤楠 via Linux-f2fs-devel" <linux-f2fs-devel@lists.sourceforge.net>
To: Chao Yu <chao@kernel.org>, "jaegeuk@kernel.org" <jaegeuk@kernel.org>
Cc: "linux-f2fs-devel@lists.sourceforge.net"
	<linux-f2fs-devel@lists.sourceforge.net>
Subject: Re: [f2fs-dev] [PATCH v2 2/2] f2fs: fix missing inplace count in overwrite with direct io
Date: Wed, 22 Sep 2021 03:01:49 +0000	[thread overview]
Message-ID: <KL1PR0601MB4003F4A5FDBD157BF9E76E22BBA29@KL1PR0601MB4003.apcprd06.prod.outlook.com> (raw)
In-Reply-To: <AJQA4AArEnKurlijneb9sapU.9.1632004451993.Hmail.changfengnan@vivo.com>


> -----Original Message-----
> From: changfengnan@vivo.com <changfengnan@vivo.com> On Behalf Of
> Chao Yu
> Sent: Sunday, September 19, 2021 6:34 AM
> To: 常凤楠 <changfengnan@vivo.com>; jaegeuk@kernel.org
> Cc: linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [PATCH v2 2/2] f2fs: fix missing inplace count in overwrite with
> direct io
> 
> On 2021/9/18 14:46, 常凤楠 wrote:
> >
> >
> >> -----Original Message-----
> >> From: changfengnan@vivo.com <changfengnan@vivo.com> On Behalf Of
> Chao
> >> Yu
> >> Sent: Friday, September 17, 2021 9:00 PM
> >> To: 常凤楠 <changfengnan@vivo.com>; jaegeuk@kernel.org
> >> Cc: linux-f2fs-devel@lists.sourceforge.net
> >> Subject: Re: [PATCH v2 2/2] f2fs: fix missing inplace count in
> >> overwrite with direct io
> >>
> >> On 2021/9/17 18:19, 常凤楠 wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: 常凤楠
> >>>> Sent: Thursday, September 16, 2021 8:46 PM
> >>>> To: Chao Yu <chao@kernel.org>; jaegeuk@kernel.org
> >>>> Cc: linux-f2fs-devel@lists.sourceforge.net
> >>>> Subject: RE: [PATCH v2 2/2] f2fs: fix missing inplace count in
> >>>> overwrite with direct io
> >>>>
> >>>>
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: changfengnan@vivo.com <changfengnan@vivo.com> On Behalf
> Of
> >>>> Chao
> >>>>> Yu
> >>>>> Sent: Thursday, September 16, 2021 8:10 PM
> >>>>> To: 常凤楠 <changfengnan@vivo.com>; jaegeuk@kernel.org
> >>>>> Cc: linux-f2fs-devel@lists.sourceforge.net
> >>>>> Subject: Re: [PATCH v2 2/2] f2fs: fix missing inplace count in
> >>>>> overwrite with direct io
> >>>>>
> >>>>> On 2021/9/16 19:30, Fengnan Chang wrote:
> >>>>>> For now, overwrite file with direct io use inplace policy, but
> >>>>>> not counted, fix it. And use stat_add_inplace_blocks(sbi, 1, )
> >>>>>> instead of stat_inc_inplace_blocks(sb, ).
> >>>>>>
> >>>>>> Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
> >>>>>> ---
> >>>>>>     fs/f2fs/data.c    | 7 ++++++-
> >>>>>>     fs/f2fs/f2fs.h    | 8 ++++----
> >>>>>>     fs/f2fs/segment.c | 2 +-
> >>>>>>     3 files changed, 11 insertions(+), 6 deletions(-)
> >>>>>>
> >>>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index
> >>>>>> c1490b9a1345..0c5728d63c33 100644
> >>>>>> --- a/fs/f2fs/data.c
> >>>>>> +++ b/fs/f2fs/data.c
> >>>>>> @@ -1491,6 +1491,9 @@ int f2fs_map_blocks(struct inode *inode,
> >>>>>> struct
> >>>>> f2fs_map_blocks *map,
> >>>>>>     		if (flag == F2FS_GET_BLOCK_DIO)
> >>>>>>     			f2fs_wait_on_block_writeback_range(inode,
> >>>>>>     						map->m_pblk, map->m_len);
> >>>>>> +		if (!f2fs_lfs_mode(sbi) && flag == F2FS_GET_BLOCK_DIO &&
> >>>>>> +				map->m_may_create)
> >>>>>> +			stat_add_inplace_blocks(sbi, map->m_len, true);
> >>>>>>     		goto out;
> >>>>>>     	}
> >>>>>>
> >>>>>> @@ -1553,7 +1556,9 @@ int f2fs_map_blocks(struct inode *inode,
> >>>>>> struct
> >>>>> f2fs_map_blocks *map,
> >>>>>>     				goto sync_out;
> >>>>>>     			blkaddr = dn.data_blkaddr;
> >>>>>>     			set_inode_flag(inode, FI_APPEND_WRITE);
> >>>>>> -		}
> >>>>>> +		} else if (!create && !f2fs_lfs_mode(sbi) && flag ==
> >>>>> F2FS_GET_BLOCK_DIO &&
> >>>>>> +				map->m_may_create)
> >>>>>
> >>>>> Why not
> >>>>>
> >>>>> } else if {!f2fs_lfs_mode(sbi) && flag == F2FS_GET_BLOCK_DIO &&
> >>>>> 					map->m_may_create)
> >>>>>
> >>>>
> >>>> You are right, no need to check create .
> >>>>
> >>> There is a problem here, if remove create check, when create file
> >>> and write with direct io, It will count in LFS and IPU both, because
> >>> preallocate block addr first. So, We still need check create.
> >>> Am I right?
> >>
> >> Could you please check below case w/ your original patch:
> >>
> >> xfs_io -f file -c "pwrite 0 8k" -c "fsync"
> >> xfs_io file -c "fpunch 0 4k"
> >> xfs_io  -c "open -d file" -c "pwrite -b 4k 0 8k"
> >>
> >> It accounts on both IPU and LFS stats.
> >
> > My origin patch is need check create:
> > @@ -1553,7 +1556,9 @@ int f2fs_map_blocks(struct inode *inode, struct
> f2fs_map_blocks *map,
> >   				goto sync_out;
> >   			blkaddr = dn.data_blkaddr;
> >   			set_inode_flag(inode, FI_APPEND_WRITE);
> > -		}
> > +		} else if (!create && !f2fs_lfs_mode(sbi) && flag ==
> F2FS_GET_BLOCK_DIO &&
> > +				map->m_may_create)
> > +			stat_add_inplace_blocks(sbi, 1, true);
> >
> > And below case looks correct, So I think check create is necessary.
> >
> > root@kvm-xfstests:/mnt/test# cat /sys/kernel/debug/f2fs/status |grep
> > SSR -C 3
> >
> >             buffer     direct   segments
> > IPU:            0          0        N/A
> > SSR:            0          0          0
> > LFS:          542          0          1
> >
> > BDF: 99, avg. vblocks: 488
> > root@kvm-xfstests:/mnt/test# xfs_io -f file -c "pwrite 0 8k" -c "fsync"
> > wrote 8192/8192 bytes at offset 0
> > 8 KiB, 2 ops; 0.0078 sec (1014.070 KiB/sec and 253.5176 ops/sec)
> > root@kvm-xfstests:/mnt/test# cat /sys/kernel/debug/f2fs/status |grep
> > SSR -C 3
> >
> >             buffer     direct   segments
> > IPU:            0          0        N/A
> > SSR:            0          0          0
> > LFS:          545          0          1
> >
> > BDF: 99, avg. vblocks: 488
> > root@kvm-xfstests:/mnt/test# xfs_io file -c "fpunch 0 4k"
> > root@kvm-xfstests:/mnt/test# cat /sys/kernel/debug/f2fs/status |grep
> > SSR -C 3
> >
> >             buffer     direct   segments
> > IPU:            0          0        N/A
> > SSR:            0          0          0
> > LFS:          545          0          1
> >
> > BDF: 99, avg. vblocks: 488
> > root@kvm-xfstests:/mnt/test# xfs_io  -c "open -d file" -c "pwrite -b 4k 0
> 8k"
> > wrote 8192/8192 bytes at offset 0
> > 8 KiB, 2 ops; 0.0322 sec (248.185 KiB/sec and 62.0463 ops/sec)
> > root@kvm-xfstests:/mnt/test# cat /sys/kernel/debug/f2fs/status |grep
> > SSR -C 3
> >
> >             buffer     direct   segments
> > IPU:            0          2        N/A
> > SSR:            0          0          0
> > LFS:          545          1          1
> 
> Shouldn't this be IPU: 1 and LFS: 1? due to [0, 4k] was a hole, and [4k, 8k]
> mapped to a valid blkaddr.
> 

Yes, you are right, we should check blkaddr is valid when physical block can found in extent cache.

Thanks.

> Thanks,
> 
> >
> > BDF: 99, avg. vblocks: 488
> >
> >
> >>
> >> Thanks,
> >>
> >>>
> >>> Thanks.
> >>>
> >>>> Thanks.
> >>>>> Thanks,
> >>>>>
> >>>>>> +			stat_add_inplace_blocks(sbi, 1, true);
> >>>>>>     	} else {
> >>>>>>     		if (create) {
> >>>>>>     			if (unlikely(f2fs_cp_error(sbi))) { diff --git
> >>>>>> a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index
> >>>>>> 3d4ee444db27..2d81e9f0a0ee
> >>>>>> 100644
> >>>>>> --- a/fs/f2fs/f2fs.h
> >>>>>> +++ b/fs/f2fs/f2fs.h
> >>>>>> @@ -3785,12 +3785,12 @@ static inline struct f2fs_stat_info
> >>>>> *F2FS_STAT(struct f2fs_sb_info *sbi)
> >>>>>>     		else								\
> >>>>>>     			((sbi)->block_count[1][(curseg)->alloc_type]++);	\
> >>>>>>     	} while (0)
> >>>>>> -#define stat_inc_inplace_blocks(sbi, direct_io)
> 	\
> >>>>>> +#define stat_add_inplace_blocks(sbi, count, direct_io)			\
> >>>>>>     	do {								\
> >>>>>>     		if (direct_io)						\
> >>>>>> -			(atomic_inc(&(sbi)->inplace_count[0]));		\
> >>>>>> +			(atomic_add(count, &(sbi)->inplace_count[0]));  \
> >>>>>>     		else								\
> >>>>>> -			(atomic_inc(&(sbi)->inplace_count[1]));		\
> >>>>>> +			(atomic_add(count, &(sbi)->inplace_count[1]));	\
> >>>>>>     	} while (0)
> >>>>>>     #define stat_update_max_atomic_write(inode)
> 	\
> >>>>>>     	do {								\
> >>>>>> @@ -3877,7 +3877,7 @@ void f2fs_update_sit_info(struct
> >>>>>> f2fs_sb_info
> >>>>> *sbi);
> >>>>>>     #define stat_inc_meta_count(sbi, blkaddr)		do { } while
> (0)
> >>>>>>     #define stat_inc_seg_type(sbi, curseg)			do { } while
> (0)
> >>>>>>     #define stat_inc_block_count(sbi, curseg)		do { } while (0)
> >>>>>> -#define stat_inc_inplace_blocks(sbi)			do { } while (0)
> >>>>>> +#define stat_add_inplace_blocks(sbi, count, direct_io)		do { }
> >> while
> >>>>> (0)
> >>>>>>     #define stat_inc_seg_count(sbi, type, gc_type)		do { } while
> >> (0)
> >>>>>>     #define stat_inc_tot_blk_count(si, blks)		do { } while (0)
> >>>>>>     #define stat_inc_data_blk_count(sbi, blks, gc_type)	do { } while
> >> (0)
> >>>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c index
> >>>>>> ded744e880d0..c542c4b687ca 100644
> >>>>>> --- a/fs/f2fs/segment.c
> >>>>>> +++ b/fs/f2fs/segment.c
> >>>>>> @@ -3611,7 +3611,7 @@ int f2fs_inplace_write_data(struct
> >>>>>> f2fs_io_info
> >>>>> *fio)
> >>>>>>     		goto drop_bio;
> >>>>>>     	}
> >>>>>>
> >>>>>> -	stat_inc_inplace_blocks(fio->sbi, false);
> >>>>>> +	stat_add_inplace_blocks(sbi, 1, false);
> >>>>>>
> >>>>>>     	if (fio->bio && !(SM_I(sbi)->ipu_policy & (1 <<
> >>>> F2FS_IPU_NOCACHE)))
> >>>>>>     		err = f2fs_merge_page_bio(fio);
> >>>>>>

_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

      parent reply	other threads:[~2021-09-22  3:02 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-16 11:30 [f2fs-dev] [PATCH v2 1/2] f2fs: separate buffer and direct io in block allocation statistics Fengnan Chang via Linux-f2fs-devel
2021-09-16 11:30 ` [f2fs-dev] [PATCH v2 2/2] f2fs: fix missing inplace count in overwrite with direct io Fengnan Chang via Linux-f2fs-devel
2021-09-16 12:09   ` Chao Yu
     [not found]   ` <AFQA*QDoEjqpHYJlWwMYT4qj.9.1631794201010.Hmail.changfengnan@vivo.com>
2021-09-16 12:45     ` 常凤楠 via Linux-f2fs-devel
2021-09-17 10:19       ` 常凤楠 via Linux-f2fs-devel
2021-09-17 12:59         ` Chao Yu
     [not found]         ` <ANgA6QClEsWrpLTMx5-VNqof.9.1631883577159.Hmail.changfengnan@vivo.com>
2021-09-18  6:46           ` 常凤楠 via Linux-f2fs-devel
2021-09-18 22:34             ` Chao Yu
     [not found]             ` <AJQA4AArEnKurlijneb9sapU.9.1632004451993.Hmail.changfengnan@vivo.com>
2021-09-22  3:01               ` 常凤楠 via Linux-f2fs-devel [this message]

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=KL1PR0601MB4003F4A5FDBD157BF9E76E22BBA29@KL1PR0601MB4003.apcprd06.prod.outlook.com \
    --to=linux-f2fs-devel@lists.sourceforge.net \
    --cc=changfengnan@vivo.com \
    --cc=chao@kernel.org \
    --cc=jaegeuk@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).