Re: [f2fs-dev] [PATCH] f2fs: remove unused stat_{inc, dec}_atomic_write

[Jaegeuk Kim <jaegeuk@kernel.org>]

On 01/14, Chao Yu wrote:
> On 2021/1/14 0:04, Jaegeuk Kim wrote:
> > On 01/13, Jack Qiu wrote:
> > > Just clean code, no logical change.
> > > 
> > > Signed-off-by: Jack Qiu <jack.qiu@huawei.com>
> > > ---
> > >   fs/f2fs/f2fs.h | 2 --
> > >   1 file changed, 2 deletions(-)
> > > 
> > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > index bb11759191dc..331e222371a3 100644
> > > --- a/fs/f2fs/f2fs.h
> > > +++ b/fs/f2fs/f2fs.h
> > > @@ -3715,8 +3715,6 @@ void f2fs_update_sit_info(struct f2fs_sb_info *sbi);
> > >   #define stat_dec_compr_inode(inode)			do { } while (0)
> > >   #define stat_add_compr_blocks(inode, blocks)		do { } while (0)
> > >   #define stat_sub_compr_blocks(inode, blocks)		do { } while (0)
> > > -#define stat_inc_atomic_write(inode)			do { } while (0)
> > > -#define stat_dec_atomic_write(inode)			do { } while (0)
> > >   #define stat_update_max_atomic_write(inode)		do { } while (0)
> > >   #define stat_inc_volatile_write(inode)			do { } while (0)
> > >   #define stat_dec_volatile_write(inode)			do { } while (0)
> > > --
> > > 2.17.1
> > 
> > 
> > Ah, it seems we need to revert the below patch.
> > 
> > Sahitya, have you seen any issue with that counter? I had to be careful before
> > merging that patch tho, that counter indicted # of active IOs, not # of files.
> 
> Jaegeuk,
> 
> stat_inc_atomic_write() is called in where we increase atomic_files, and
> stat_dec_atomic_write() is called in where we decrease atomic_files, so,
> any difference in between these two stats?

Ah, I got the point back. :) I was looking for # of inmem pages.

> 
> And now f2fs_drop_inmem_pages() is lockless, will potential concurrent invoking
> can cause incorrect aw_cnt? e.g.
> 
> ThreadA					ThreadB
> - f2fs_drop_inmem_pages			- f2fs_drop_inmem_pages
>  - stat_dec_atomic_write()
> 					 - stat_dec_atomic_write()
> 
> Thanks,
> 
> > 
> > > From 4b3245a1dceb550ad643a3ecd831a3147d1a6f9f Mon Sep 17 00:00:00 2001
> > From: Jaegeuk Kim <jaegeuk@kernel.org>
> > Date: Wed, 13 Jan 2021 07:49:11 -0800
> > Subject: [PATCH] Revert "f2fs: cleanup duplicate stats for atomic files"
> > 
> > This reverts commit 0e6d01643c207fdcd77b9b40c29cbe1c63f03c15.
> > 
> > The counter meant # of atomic writes on the fly.
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >   fs/f2fs/debug.c   | 3 ++-
> >   fs/f2fs/f2fs.h    | 7 ++++++-
> >   fs/f2fs/file.c    | 1 +
> >   fs/f2fs/segment.c | 1 +
> >   4 files changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/f2fs/debug.c b/fs/f2fs/debug.c
> > index 197c914119da..f55d64ce61d6 100644
> > --- a/fs/f2fs/debug.c
> > +++ b/fs/f2fs/debug.c
> > @@ -92,7 +92,7 @@ static void update_general_status(struct f2fs_sb_info *sbi)
> >   	si->nquota_files = sbi->nquota_files;
> >   	si->ndirty_all = sbi->ndirty_inode[DIRTY_META];
> >   	si->inmem_pages = get_pages(sbi, F2FS_INMEM_PAGES);
> > -	si->aw_cnt = sbi->atomic_files;
> > +	si->aw_cnt = atomic_read(&sbi->aw_cnt);
> >   	si->vw_cnt = atomic_read(&sbi->vw_cnt);
> >   	si->max_aw_cnt = atomic_read(&sbi->max_aw_cnt);
> >   	si->max_vw_cnt = atomic_read(&sbi->max_vw_cnt);
> > @@ -556,6 +556,7 @@ int f2fs_build_stats(struct f2fs_sb_info *sbi)
> >   	for (i = META_CP; i < META_MAX; i++)
> >   		atomic_set(&sbi->meta_count[i], 0);
> > +	atomic_set(&sbi->aw_cnt, 0);
> >   	atomic_set(&sbi->vw_cnt, 0);
> >   	atomic_set(&sbi->max_aw_cnt, 0);
> >   	atomic_set(&sbi->max_vw_cnt, 0);
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index 63852404151e..88356dbe7540 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -1549,6 +1549,7 @@ struct f2fs_sb_info {
> >   	atomic_t inline_dir;			/* # of inline_dentry inodes */
> >   	atomic_t compr_inode;			/* # of compressed inodes */
> >   	atomic64_t compr_blocks;		/* # of compressed blocks */
> > +	atomic_t aw_cnt;			/* # of atomic writes */
> >   	atomic_t vw_cnt;			/* # of volatile writes */
> >   	atomic_t max_aw_cnt;			/* max # of atomic writes */
> >   	atomic_t max_vw_cnt;			/* max # of volatile writes */
> > @@ -3670,9 +3671,13 @@ static inline struct f2fs_stat_info *F2FS_STAT(struct f2fs_sb_info *sbi)
> >   		((sbi)->block_count[(curseg)->alloc_type]++)
> >   #define stat_inc_inplace_blocks(sbi)					\
> >   		(atomic_inc(&(sbi)->inplace_count))
> > +#define stat_inc_atomic_write(inode)					\
> > +		(atomic_inc(&F2FS_I_SB(inode)->aw_cnt))
> > +#define stat_dec_atomic_write(inode)					\
> > +		(atomic_dec(&F2FS_I_SB(inode)->aw_cnt))
> >   #define stat_update_max_atomic_write(inode)				\
> >   	do {								\
> > -		int cur = F2FS_I_SB(inode)->atomic_files;	\
> > +		int cur = atomic_read(&F2FS_I_SB(inode)->aw_cnt);	\
> >   		int max = atomic_read(&F2FS_I_SB(inode)->max_aw_cnt);	\
> >   		if (cur > max)						\
> >   			atomic_set(&F2FS_I_SB(inode)->max_aw_cnt, cur);	\
> > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > index e3a5b620b50a..c6e96c094b29 100644
> > --- a/fs/f2fs/file.c
> > +++ b/fs/f2fs/file.c
> > @@ -2077,6 +2077,7 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
> >   	f2fs_update_time(F2FS_I_SB(inode), REQ_TIME);
> >   	F2FS_I(inode)->inmem_task = current;
> > +	stat_inc_atomic_write(inode);
> >   	stat_update_max_atomic_write(inode);
> >   out:
> >   	inode_unlock(inode);
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index deca74cb17df..7fbb2a31bd01 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -335,6 +335,7 @@ void f2fs_drop_inmem_pages(struct inode *inode)
> >   	}
> >   	fi->i_gc_failures[GC_FAILURE_ATOMIC] = 0;
> > +	stat_dec_atomic_write(inode);
> >   	spin_lock(&sbi->inode_lock[ATOMIC_FILE]);
> >   	if (!list_empty(&fi->inmem_ilist))
> > 


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