[PATCH] f2fs: prevent atomic file from being dirtied before commit

[PATCH] f2fs: prevent atomic file from being dirtied before commit

[Daeho Jeong]

From: Daeho Jeong <daehojeong@google.com>

Keep atomic file clean while updating and make it dirtied during commit
in order to avoid unnecessary and excessive inode updates in the previous
fix.

Fixes: 4bf78322346f ("f2fs: mark inode dirty for FI_ATOMIC_COMMITTED flag")
Signed-off-by: Daeho Jeong <daehojeong@google.com>
---
 fs/f2fs/f2fs.h    |  3 +--
 fs/f2fs/inode.c   | 10 ++++++----
 fs/f2fs/segment.c | 10 ++++++++--
 3 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 465b2fd50c70..5a7f6fa8b585 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -801,7 +801,7 @@ enum {
 	FI_COMPRESS_RELEASED,	/* compressed blocks were released */
 	FI_ALIGNED_WRITE,	/* enable aligned write */
 	FI_COW_FILE,		/* indicate COW file */
-	FI_ATOMIC_COMMITTED,	/* indicate atomic commit completed except disk sync */
+	FI_ATOMIC_DIRTIED,	/* indicate atomic file is dirtied */
 	FI_ATOMIC_REPLACE,	/* indicate atomic replace */
 	FI_OPENED_FILE,		/* indicate file has been opened */
 	FI_MAX,			/* max flag, never be used */
@@ -3042,7 +3042,6 @@ static inline void __mark_inode_dirty_flag(struct inode *inode,
 	case FI_INLINE_DOTS:
 	case FI_PIN_FILE:
 	case FI_COMPRESS_RELEASED:
-	case FI_ATOMIC_COMMITTED:
 		f2fs_mark_inode_dirty_sync(inode, true);
 	}
 }
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index 1eb250c6b392..5dd3e55d2be2 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -35,6 +35,11 @@ void f2fs_mark_inode_dirty_sync(struct inode *inode, bool sync)
 	if (f2fs_inode_dirtied(inode, sync))
 		return;
 
+	if (f2fs_is_atomic_file(inode)) {
+		set_inode_flag(inode, FI_ATOMIC_DIRTIED);
+		return;
+	}
+
 	mark_inode_dirty_sync(inode);
 }
 
@@ -653,10 +658,7 @@ void f2fs_update_inode(struct inode *inode, struct page *node_page)
 	ri->i_gid = cpu_to_le32(i_gid_read(inode));
 	ri->i_links = cpu_to_le32(inode->i_nlink);
 	ri->i_blocks = cpu_to_le64(SECTOR_TO_BLOCK(inode->i_blocks) + 1);
-
-	if (!f2fs_is_atomic_file(inode) ||
-			is_inode_flag_set(inode, FI_ATOMIC_COMMITTED))
-		ri->i_size = cpu_to_le64(i_size_read(inode));
+	ri->i_size = cpu_to_le64(i_size_read(inode));
 
 	if (et) {
 		read_lock(&et->lock);
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 78c3198a6308..2b5391b229a8 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -196,9 +196,12 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
 		truncate_inode_pages_final(inode->i_mapping);
 
 	release_atomic_write_cnt(inode);
-	clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
 	clear_inode_flag(inode, FI_ATOMIC_REPLACE);
 	clear_inode_flag(inode, FI_ATOMIC_FILE);
+	if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) {
+		clear_inode_flag(inode, FI_ATOMIC_DIRTIED);
+		f2fs_mark_inode_dirty_sync(inode, true);
+	}
 	stat_dec_atomic_inode(inode);
 
 	F2FS_I(inode)->atomic_write_task = NULL;
@@ -365,7 +368,10 @@ static int __f2fs_commit_atomic_write(struct inode *inode)
 		sbi->revoked_atomic_block += fi->atomic_write_cnt;
 	} else {
 		sbi->committed_atomic_block += fi->atomic_write_cnt;
-		set_inode_flag(inode, FI_ATOMIC_COMMITTED);
+		if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) {
+			clear_inode_flag(inode, FI_ATOMIC_DIRTIED);
+			f2fs_mark_inode_dirty_sync(inode, true);
+		}
 	}
 
 	__complete_revoke_list(inode, &revoke_list, ret ? true : false);
-- 
2.46.0.295.g3b9ea8a38a-goog

Re: [f2fs-dev] [PATCH] f2fs: prevent atomic file from being dirtied before commit

[patchwork-bot+f2fs]

Hello:

This patch was applied to jaegeuk/f2fs.git (dev)
by Jaegeuk Kim <jaegeuk@kernel.org>:

On Mon, 26 Aug 2024 13:23:52 -0700 you wrote:
> From: Daeho Jeong <daehojeong@google.com>
> 
> Keep atomic file clean while updating and make it dirtied during commit
> in order to avoid unnecessary and excessive inode updates in the previous
> fix.
> 
> Fixes: 4bf78322346f ("f2fs: mark inode dirty for FI_ATOMIC_COMMITTED flag")
> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> 
> [...]

Here is the summary with links:
  - [f2fs-dev] f2fs: prevent atomic file from being dirtied before commit
    https://git.kernel.org/jaegeuk/f2fs/c/a433a8f0eb31

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [f2fs-dev] [PATCH] f2fs: prevent atomic file from being dirtied before commit

[Chao Yu]

On 2024/8/27 4:23, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@google.com>
> 
> Keep atomic file clean while updating and make it dirtied during commit
> in order to avoid unnecessary and excessive inode updates in the previous
> fix.
> 
> Fixes: 4bf78322346f ("f2fs: mark inode dirty for FI_ATOMIC_COMMITTED flag")
> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> ---
>   fs/f2fs/f2fs.h    |  3 +--
>   fs/f2fs/inode.c   | 10 ++++++----
>   fs/f2fs/segment.c | 10 ++++++++--
>   3 files changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 465b2fd50c70..5a7f6fa8b585 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -801,7 +801,7 @@ enum {
>   	FI_COMPRESS_RELEASED,	/* compressed blocks were released */
>   	FI_ALIGNED_WRITE,	/* enable aligned write */
>   	FI_COW_FILE,		/* indicate COW file */
> -	FI_ATOMIC_COMMITTED,	/* indicate atomic commit completed except disk sync */
> +	FI_ATOMIC_DIRTIED,	/* indicate atomic file is dirtied */
>   	FI_ATOMIC_REPLACE,	/* indicate atomic replace */
>   	FI_OPENED_FILE,		/* indicate file has been opened */
>   	FI_MAX,			/* max flag, never be used */
> @@ -3042,7 +3042,6 @@ static inline void __mark_inode_dirty_flag(struct inode *inode,
>   	case FI_INLINE_DOTS:
>   	case FI_PIN_FILE:
>   	case FI_COMPRESS_RELEASED:
> -	case FI_ATOMIC_COMMITTED:
>   		f2fs_mark_inode_dirty_sync(inode, true);
>   	}
>   }
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 1eb250c6b392..5dd3e55d2be2 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -35,6 +35,11 @@ void f2fs_mark_inode_dirty_sync(struct inode *inode, bool sync)
>   	if (f2fs_inode_dirtied(inode, sync))

It will return directly here if inode was dirtied, so it may missed to set
FI_ATOMIC_DIRTIED flag?

Thanks,

>   		return;
>   
> +	if (f2fs_is_atomic_file(inode)) {
> +		set_inode_flag(inode, FI_ATOMIC_DIRTIED);
> +		return;
> +	}
> +
>   	mark_inode_dirty_sync(inode);
>   }
>   
> @@ -653,10 +658,7 @@ void f2fs_update_inode(struct inode *inode, struct page *node_page)
>   	ri->i_gid = cpu_to_le32(i_gid_read(inode));
>   	ri->i_links = cpu_to_le32(inode->i_nlink);
>   	ri->i_blocks = cpu_to_le64(SECTOR_TO_BLOCK(inode->i_blocks) + 1);
> -
> -	if (!f2fs_is_atomic_file(inode) ||
> -			is_inode_flag_set(inode, FI_ATOMIC_COMMITTED))
> -		ri->i_size = cpu_to_le64(i_size_read(inode));
> +	ri->i_size = cpu_to_le64(i_size_read(inode));
>   
>   	if (et) {
>   		read_lock(&et->lock);
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 78c3198a6308..2b5391b229a8 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -196,9 +196,12 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
>   		truncate_inode_pages_final(inode->i_mapping);
>   
>   	release_atomic_write_cnt(inode);
> -	clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
>   	clear_inode_flag(inode, FI_ATOMIC_REPLACE);
>   	clear_inode_flag(inode, FI_ATOMIC_FILE);
> +	if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) {
> +		clear_inode_flag(inode, FI_ATOMIC_DIRTIED);
> +		f2fs_mark_inode_dirty_sync(inode, true);
> +	}
>   	stat_dec_atomic_inode(inode);
>   
>   	F2FS_I(inode)->atomic_write_task = NULL;
> @@ -365,7 +368,10 @@ static int __f2fs_commit_atomic_write(struct inode *inode)
>   		sbi->revoked_atomic_block += fi->atomic_write_cnt;
>   	} else {
>   		sbi->committed_atomic_block += fi->atomic_write_cnt;
> -		set_inode_flag(inode, FI_ATOMIC_COMMITTED);
> +		if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) {
> +			clear_inode_flag(inode, FI_ATOMIC_DIRTIED);
> +			f2fs_mark_inode_dirty_sync(inode, true);
> +		}
>   	}
>   
>   	__complete_revoke_list(inode, &revoke_list, ret ? true : false);

Re: [f2fs-dev] [PATCH] f2fs: prevent atomic file from being dirtied before commit

[Daeho Jeong]

On Mon, Sep 2, 2024 at 3:08 AM Chao Yu <chao@kernel.org> wrote:
>
> On 2024/8/27 4:23, Daeho Jeong wrote:
> > From: Daeho Jeong <daehojeong@google.com>
> >
> > Keep atomic file clean while updating and make it dirtied during commit
> > in order to avoid unnecessary and excessive inode updates in the previous
> > fix.
> >
> > Fixes: 4bf78322346f ("f2fs: mark inode dirty for FI_ATOMIC_COMMITTED flag")
> > Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > ---
> >   fs/f2fs/f2fs.h    |  3 +--
> >   fs/f2fs/inode.c   | 10 ++++++----
> >   fs/f2fs/segment.c | 10 ++++++++--
> >   3 files changed, 15 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index 465b2fd50c70..5a7f6fa8b585 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -801,7 +801,7 @@ enum {
> >       FI_COMPRESS_RELEASED,   /* compressed blocks were released */
> >       FI_ALIGNED_WRITE,       /* enable aligned write */
> >       FI_COW_FILE,            /* indicate COW file */
> > -     FI_ATOMIC_COMMITTED,    /* indicate atomic commit completed except disk sync */
> > +     FI_ATOMIC_DIRTIED,      /* indicate atomic file is dirtied */
> >       FI_ATOMIC_REPLACE,      /* indicate atomic replace */
> >       FI_OPENED_FILE,         /* indicate file has been opened */
> >       FI_MAX,                 /* max flag, never be used */
> > @@ -3042,7 +3042,6 @@ static inline void __mark_inode_dirty_flag(struct inode *inode,
> >       case FI_INLINE_DOTS:
> >       case FI_PIN_FILE:
> >       case FI_COMPRESS_RELEASED:
> > -     case FI_ATOMIC_COMMITTED:
> >               f2fs_mark_inode_dirty_sync(inode, true);
> >       }
> >   }
> > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > index 1eb250c6b392..5dd3e55d2be2 100644
> > --- a/fs/f2fs/inode.c
> > +++ b/fs/f2fs/inode.c
> > @@ -35,6 +35,11 @@ void f2fs_mark_inode_dirty_sync(struct inode *inode, bool sync)
> >       if (f2fs_inode_dirtied(inode, sync))
>
> It will return directly here if inode was dirtied, so it may missed to set
> FI_ATOMIC_DIRTIED flag?

Is it possible for it to be already dirty, since we already made it
clean with f2fs_write_inode() when we started the atomic write?

>
> Thanks,
>
> >               return;
> >
> > +     if (f2fs_is_atomic_file(inode)) {
> > +             set_inode_flag(inode, FI_ATOMIC_DIRTIED);
> > +             return;
> > +     }
> > +
> >       mark_inode_dirty_sync(inode);
> >   }
> >
> > @@ -653,10 +658,7 @@ void f2fs_update_inode(struct inode *inode, struct page *node_page)
> >       ri->i_gid = cpu_to_le32(i_gid_read(inode));
> >       ri->i_links = cpu_to_le32(inode->i_nlink);
> >       ri->i_blocks = cpu_to_le64(SECTOR_TO_BLOCK(inode->i_blocks) + 1);
> > -
> > -     if (!f2fs_is_atomic_file(inode) ||
> > -                     is_inode_flag_set(inode, FI_ATOMIC_COMMITTED))
> > -             ri->i_size = cpu_to_le64(i_size_read(inode));
> > +     ri->i_size = cpu_to_le64(i_size_read(inode));
> >
> >       if (et) {
> >               read_lock(&et->lock);
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index 78c3198a6308..2b5391b229a8 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -196,9 +196,12 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
> >               truncate_inode_pages_final(inode->i_mapping);
> >
> >       release_atomic_write_cnt(inode);
> > -     clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
> >       clear_inode_flag(inode, FI_ATOMIC_REPLACE);
> >       clear_inode_flag(inode, FI_ATOMIC_FILE);
> > +     if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) {
> > +             clear_inode_flag(inode, FI_ATOMIC_DIRTIED);
> > +             f2fs_mark_inode_dirty_sync(inode, true);
> > +     }
> >       stat_dec_atomic_inode(inode);
> >
> >       F2FS_I(inode)->atomic_write_task = NULL;
> > @@ -365,7 +368,10 @@ static int __f2fs_commit_atomic_write(struct inode *inode)
> >               sbi->revoked_atomic_block += fi->atomic_write_cnt;
> >       } else {
> >               sbi->committed_atomic_block += fi->atomic_write_cnt;
> > -             set_inode_flag(inode, FI_ATOMIC_COMMITTED);
> > +             if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) {
> > +                     clear_inode_flag(inode, FI_ATOMIC_DIRTIED);
> > +                     f2fs_mark_inode_dirty_sync(inode, true);
> > +             }
> >       }
> >
> >       __complete_revoke_list(inode, &revoke_list, ret ? true : false);
>

Re: [f2fs-dev] [PATCH] f2fs: prevent atomic file from being dirtied before commit

[Chao Yu]

On 2024/9/4 1:07, Daeho Jeong wrote:
> On Mon, Sep 2, 2024 at 3:08 AM Chao Yu <chao@kernel.org> wrote:
>>
>> On 2024/8/27 4:23, Daeho Jeong wrote:
>>> From: Daeho Jeong <daehojeong@google.com>
>>>
>>> Keep atomic file clean while updating and make it dirtied during commit
>>> in order to avoid unnecessary and excessive inode updates in the previous
>>> fix.
>>>
>>> Fixes: 4bf78322346f ("f2fs: mark inode dirty for FI_ATOMIC_COMMITTED flag")
>>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
>>> ---
>>>    fs/f2fs/f2fs.h    |  3 +--
>>>    fs/f2fs/inode.c   | 10 ++++++----
>>>    fs/f2fs/segment.c | 10 ++++++++--
>>>    3 files changed, 15 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>> index 465b2fd50c70..5a7f6fa8b585 100644
>>> --- a/fs/f2fs/f2fs.h
>>> +++ b/fs/f2fs/f2fs.h
>>> @@ -801,7 +801,7 @@ enum {
>>>        FI_COMPRESS_RELEASED,   /* compressed blocks were released */
>>>        FI_ALIGNED_WRITE,       /* enable aligned write */
>>>        FI_COW_FILE,            /* indicate COW file */
>>> -     FI_ATOMIC_COMMITTED,    /* indicate atomic commit completed except disk sync */
>>> +     FI_ATOMIC_DIRTIED,      /* indicate atomic file is dirtied */
>>>        FI_ATOMIC_REPLACE,      /* indicate atomic replace */
>>>        FI_OPENED_FILE,         /* indicate file has been opened */
>>>        FI_MAX,                 /* max flag, never be used */
>>> @@ -3042,7 +3042,6 @@ static inline void __mark_inode_dirty_flag(struct inode *inode,
>>>        case FI_INLINE_DOTS:
>>>        case FI_PIN_FILE:
>>>        case FI_COMPRESS_RELEASED:
>>> -     case FI_ATOMIC_COMMITTED:
>>>                f2fs_mark_inode_dirty_sync(inode, true);
>>>        }
>>>    }
>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>> index 1eb250c6b392..5dd3e55d2be2 100644
>>> --- a/fs/f2fs/inode.c
>>> +++ b/fs/f2fs/inode.c
>>> @@ -35,6 +35,11 @@ void f2fs_mark_inode_dirty_sync(struct inode *inode, bool sync)
>>>        if (f2fs_inode_dirtied(inode, sync))
>>
>> It will return directly here if inode was dirtied, so it may missed to set
>> FI_ATOMIC_DIRTIED flag?
> 
> Is it possible for it to be already dirty, since we already made it
> clean with f2fs_write_inode() when we started the atomic write?

Some ioctl interfaces may race w/ atomic write? e.g. set_pin_file won't
check atomic_file status, and may dirty inode after we started atomic
write, so we'd better detect such race condition and break ioctl to
avoid ruin atomic write? and maybe we can add f2fs_bug_on() in
f2fs_mark_inode_dirty_sync() to detect any other missing cases?

Thanks,

> 
>>
>> Thanks,
>>
>>>                return;
>>>
>>> +     if (f2fs_is_atomic_file(inode)) {
>>> +             set_inode_flag(inode, FI_ATOMIC_DIRTIED);
>>> +             return;
>>> +     }
>>> +
>>>        mark_inode_dirty_sync(inode);
>>>    }
>>>
>>> @@ -653,10 +658,7 @@ void f2fs_update_inode(struct inode *inode, struct page *node_page)
>>>        ri->i_gid = cpu_to_le32(i_gid_read(inode));
>>>        ri->i_links = cpu_to_le32(inode->i_nlink);
>>>        ri->i_blocks = cpu_to_le64(SECTOR_TO_BLOCK(inode->i_blocks) + 1);
>>> -
>>> -     if (!f2fs_is_atomic_file(inode) ||
>>> -                     is_inode_flag_set(inode, FI_ATOMIC_COMMITTED))
>>> -             ri->i_size = cpu_to_le64(i_size_read(inode));
>>> +     ri->i_size = cpu_to_le64(i_size_read(inode));
>>>
>>>        if (et) {
>>>                read_lock(&et->lock);
>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>> index 78c3198a6308..2b5391b229a8 100644
>>> --- a/fs/f2fs/segment.c
>>> +++ b/fs/f2fs/segment.c
>>> @@ -196,9 +196,12 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
>>>                truncate_inode_pages_final(inode->i_mapping);
>>>
>>>        release_atomic_write_cnt(inode);
>>> -     clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
>>>        clear_inode_flag(inode, FI_ATOMIC_REPLACE);
>>>        clear_inode_flag(inode, FI_ATOMIC_FILE);
>>> +     if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) {
>>> +             clear_inode_flag(inode, FI_ATOMIC_DIRTIED);
>>> +             f2fs_mark_inode_dirty_sync(inode, true);
>>> +     }
>>>        stat_dec_atomic_inode(inode);
>>>
>>>        F2FS_I(inode)->atomic_write_task = NULL;
>>> @@ -365,7 +368,10 @@ static int __f2fs_commit_atomic_write(struct inode *inode)
>>>                sbi->revoked_atomic_block += fi->atomic_write_cnt;
>>>        } else {
>>>                sbi->committed_atomic_block += fi->atomic_write_cnt;
>>> -             set_inode_flag(inode, FI_ATOMIC_COMMITTED);
>>> +             if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) {
>>> +                     clear_inode_flag(inode, FI_ATOMIC_DIRTIED);
>>> +                     f2fs_mark_inode_dirty_sync(inode, true);
>>> +             }
>>>        }
>>>
>>>        __complete_revoke_list(inode, &revoke_list, ret ? true : false);
>>

Re: [f2fs-dev] [PATCH] f2fs: prevent atomic file from being dirtied before commit

[Daeho Jeong]

On Tue, Sep 3, 2024 at 7:26 PM Chao Yu <chao@kernel.org> wrote:
>
> On 2024/9/4 1:07, Daeho Jeong wrote:
> > On Mon, Sep 2, 2024 at 3:08 AM Chao Yu <chao@kernel.org> wrote:
> >>
> >> On 2024/8/27 4:23, Daeho Jeong wrote:
> >>> From: Daeho Jeong <daehojeong@google.com>
> >>>
> >>> Keep atomic file clean while updating and make it dirtied during commit
> >>> in order to avoid unnecessary and excessive inode updates in the previous
> >>> fix.
> >>>
> >>> Fixes: 4bf78322346f ("f2fs: mark inode dirty for FI_ATOMIC_COMMITTED flag")
> >>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> >>> ---
> >>>    fs/f2fs/f2fs.h    |  3 +--
> >>>    fs/f2fs/inode.c   | 10 ++++++----
> >>>    fs/f2fs/segment.c | 10 ++++++++--
> >>>    3 files changed, 15 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> >>> index 465b2fd50c70..5a7f6fa8b585 100644
> >>> --- a/fs/f2fs/f2fs.h
> >>> +++ b/fs/f2fs/f2fs.h
> >>> @@ -801,7 +801,7 @@ enum {
> >>>        FI_COMPRESS_RELEASED,   /* compressed blocks were released */
> >>>        FI_ALIGNED_WRITE,       /* enable aligned write */
> >>>        FI_COW_FILE,            /* indicate COW file */
> >>> -     FI_ATOMIC_COMMITTED,    /* indicate atomic commit completed except disk sync */
> >>> +     FI_ATOMIC_DIRTIED,      /* indicate atomic file is dirtied */
> >>>        FI_ATOMIC_REPLACE,      /* indicate atomic replace */
> >>>        FI_OPENED_FILE,         /* indicate file has been opened */
> >>>        FI_MAX,                 /* max flag, never be used */
> >>> @@ -3042,7 +3042,6 @@ static inline void __mark_inode_dirty_flag(struct inode *inode,
> >>>        case FI_INLINE_DOTS:
> >>>        case FI_PIN_FILE:
> >>>        case FI_COMPRESS_RELEASED:
> >>> -     case FI_ATOMIC_COMMITTED:
> >>>                f2fs_mark_inode_dirty_sync(inode, true);
> >>>        }
> >>>    }
> >>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> >>> index 1eb250c6b392..5dd3e55d2be2 100644
> >>> --- a/fs/f2fs/inode.c
> >>> +++ b/fs/f2fs/inode.c
> >>> @@ -35,6 +35,11 @@ void f2fs_mark_inode_dirty_sync(struct inode *inode, bool sync)
> >>>        if (f2fs_inode_dirtied(inode, sync))
> >>
> >> It will return directly here if inode was dirtied, so it may missed to set
> >> FI_ATOMIC_DIRTIED flag?
> >
> > Is it possible for it to be already dirty, since we already made it
> > clean with f2fs_write_inode() when we started the atomic write?
>
> Some ioctl interfaces may race w/ atomic write? e.g. set_pin_file won't
> check atomic_file status, and may dirty inode after we started atomic
> write, so we'd better detect such race condition and break ioctl to
> avoid ruin atomic write? and maybe we can add f2fs_bug_on() in
> f2fs_mark_inode_dirty_sync() to detect any other missing cases?
>

How about exchanging the positions of f2fs_write_inode() and
set_inode_flag() in f2fs_ioc_start_atomic_write()?

...
        f2fs_write_inode(inode, NULL);

        stat_inc_atomic_inode(inode);

        set_inode_flag(inode, FI_ATOMIC_FILE);
...

> Thanks,
>
> >
> >>
> >> Thanks,
> >>
> >>>                return;
> >>>
> >>> +     if (f2fs_is_atomic_file(inode)) {
> >>> +             set_inode_flag(inode, FI_ATOMIC_DIRTIED);
> >>> +             return;
> >>> +     }
> >>> +
> >>>        mark_inode_dirty_sync(inode);
> >>>    }
> >>>
> >>> @@ -653,10 +658,7 @@ void f2fs_update_inode(struct inode *inode, struct page *node_page)
> >>>        ri->i_gid = cpu_to_le32(i_gid_read(inode));
> >>>        ri->i_links = cpu_to_le32(inode->i_nlink);
> >>>        ri->i_blocks = cpu_to_le64(SECTOR_TO_BLOCK(inode->i_blocks) + 1);
> >>> -
> >>> -     if (!f2fs_is_atomic_file(inode) ||
> >>> -                     is_inode_flag_set(inode, FI_ATOMIC_COMMITTED))
> >>> -             ri->i_size = cpu_to_le64(i_size_read(inode));
> >>> +     ri->i_size = cpu_to_le64(i_size_read(inode));
> >>>
> >>>        if (et) {
> >>>                read_lock(&et->lock);
> >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> >>> index 78c3198a6308..2b5391b229a8 100644
> >>> --- a/fs/f2fs/segment.c
> >>> +++ b/fs/f2fs/segment.c
> >>> @@ -196,9 +196,12 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
> >>>                truncate_inode_pages_final(inode->i_mapping);
> >>>
> >>>        release_atomic_write_cnt(inode);
> >>> -     clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
> >>>        clear_inode_flag(inode, FI_ATOMIC_REPLACE);
> >>>        clear_inode_flag(inode, FI_ATOMIC_FILE);
> >>> +     if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) {
> >>> +             clear_inode_flag(inode, FI_ATOMIC_DIRTIED);
> >>> +             f2fs_mark_inode_dirty_sync(inode, true);
> >>> +     }
> >>>        stat_dec_atomic_inode(inode);
> >>>
> >>>        F2FS_I(inode)->atomic_write_task = NULL;
> >>> @@ -365,7 +368,10 @@ static int __f2fs_commit_atomic_write(struct inode *inode)
> >>>                sbi->revoked_atomic_block += fi->atomic_write_cnt;
> >>>        } else {
> >>>                sbi->committed_atomic_block += fi->atomic_write_cnt;
> >>> -             set_inode_flag(inode, FI_ATOMIC_COMMITTED);
> >>> +             if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) {
> >>> +                     clear_inode_flag(inode, FI_ATOMIC_DIRTIED);
> >>> +                     f2fs_mark_inode_dirty_sync(inode, true);
> >>> +             }
> >>>        }
> >>>
> >>>        __complete_revoke_list(inode, &revoke_list, ret ? true : false);
> >>
>

Re: [f2fs-dev] [PATCH] f2fs: prevent atomic file from being dirtied before commit

[Chao Yu]

On 2024/9/4 10:52, Daeho Jeong wrote:
> On Tue, Sep 3, 2024 at 7:26 PM Chao Yu <chao@kernel.org> wrote:
>>
>> On 2024/9/4 1:07, Daeho Jeong wrote:
>>> On Mon, Sep 2, 2024 at 3:08 AM Chao Yu <chao@kernel.org> wrote:
>>>>
>>>> On 2024/8/27 4:23, Daeho Jeong wrote:
>>>>> From: Daeho Jeong <daehojeong@google.com>
>>>>>
>>>>> Keep atomic file clean while updating and make it dirtied during commit
>>>>> in order to avoid unnecessary and excessive inode updates in the previous
>>>>> fix.
>>>>>
>>>>> Fixes: 4bf78322346f ("f2fs: mark inode dirty for FI_ATOMIC_COMMITTED flag")
>>>>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
>>>>> ---
>>>>>     fs/f2fs/f2fs.h    |  3 +--
>>>>>     fs/f2fs/inode.c   | 10 ++++++----
>>>>>     fs/f2fs/segment.c | 10 ++++++++--
>>>>>     3 files changed, 15 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>>> index 465b2fd50c70..5a7f6fa8b585 100644
>>>>> --- a/fs/f2fs/f2fs.h
>>>>> +++ b/fs/f2fs/f2fs.h
>>>>> @@ -801,7 +801,7 @@ enum {
>>>>>         FI_COMPRESS_RELEASED,   /* compressed blocks were released */
>>>>>         FI_ALIGNED_WRITE,       /* enable aligned write */
>>>>>         FI_COW_FILE,            /* indicate COW file */
>>>>> -     FI_ATOMIC_COMMITTED,    /* indicate atomic commit completed except disk sync */
>>>>> +     FI_ATOMIC_DIRTIED,      /* indicate atomic file is dirtied */
>>>>>         FI_ATOMIC_REPLACE,      /* indicate atomic replace */
>>>>>         FI_OPENED_FILE,         /* indicate file has been opened */
>>>>>         FI_MAX,                 /* max flag, never be used */
>>>>> @@ -3042,7 +3042,6 @@ static inline void __mark_inode_dirty_flag(struct inode *inode,
>>>>>         case FI_INLINE_DOTS:
>>>>>         case FI_PIN_FILE:
>>>>>         case FI_COMPRESS_RELEASED:
>>>>> -     case FI_ATOMIC_COMMITTED:
>>>>>                 f2fs_mark_inode_dirty_sync(inode, true);
>>>>>         }
>>>>>     }
>>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>>>> index 1eb250c6b392..5dd3e55d2be2 100644
>>>>> --- a/fs/f2fs/inode.c
>>>>> +++ b/fs/f2fs/inode.c
>>>>> @@ -35,6 +35,11 @@ void f2fs_mark_inode_dirty_sync(struct inode *inode, bool sync)
>>>>>         if (f2fs_inode_dirtied(inode, sync))
>>>>
>>>> It will return directly here if inode was dirtied, so it may missed to set
>>>> FI_ATOMIC_DIRTIED flag?
>>>
>>> Is it possible for it to be already dirty, since we already made it
>>> clean with f2fs_write_inode() when we started the atomic write?
>>
>> Some ioctl interfaces may race w/ atomic write? e.g. set_pin_file won't
>> check atomic_file status, and may dirty inode after we started atomic
>> write, so we'd better detect such race condition and break ioctl to
>> avoid ruin atomic write? and maybe we can add f2fs_bug_on() in
>> f2fs_mark_inode_dirty_sync() to detect any other missing cases?
>>
> 
> How about exchanging the positions of f2fs_write_inode() and
> set_inode_flag() in f2fs_ioc_start_atomic_write()?
> 
> ...
>          f2fs_write_inode(inode, NULL);
> 
>          stat_inc_atomic_inode(inode);
> 
>          set_inode_flag(inode, FI_ATOMIC_FILE);
> ...

Oh, I'm not sure I've got your point, after exchanging we still may suffer
below race condition, right?

- f2fs_ioc_start_atomic_write
  - set_inode_flag(inode, FI_ATOMIC_FILE)
  - f2fs_write_inode(inode, NULL)
					- f2fs_ioc_set_pin_file
					 - set_inode_flag(inode, FI_PIN_FILE)
					  - __mark_inode_dirty_flag()
- f2fs_ioc_commit_atomic_write

So that I proposed a fix for this:
https://lore.kernel.org/linux-f2fs-devel/20240904032047.1264706-1-chao@kernel.org

Thanks,

> 
>> Thanks,
>>
>>>
>>>>
>>>> Thanks,
>>>>
>>>>>                 return;
>>>>>
>>>>> +     if (f2fs_is_atomic_file(inode)) {
>>>>> +             set_inode_flag(inode, FI_ATOMIC_DIRTIED);
>>>>> +             return;
>>>>> +     }
>>>>> +
>>>>>         mark_inode_dirty_sync(inode);
>>>>>     }
>>>>>
>>>>> @@ -653,10 +658,7 @@ void f2fs_update_inode(struct inode *inode, struct page *node_page)
>>>>>         ri->i_gid = cpu_to_le32(i_gid_read(inode));
>>>>>         ri->i_links = cpu_to_le32(inode->i_nlink);
>>>>>         ri->i_blocks = cpu_to_le64(SECTOR_TO_BLOCK(inode->i_blocks) + 1);
>>>>> -
>>>>> -     if (!f2fs_is_atomic_file(inode) ||
>>>>> -                     is_inode_flag_set(inode, FI_ATOMIC_COMMITTED))
>>>>> -             ri->i_size = cpu_to_le64(i_size_read(inode));
>>>>> +     ri->i_size = cpu_to_le64(i_size_read(inode));
>>>>>
>>>>>         if (et) {
>>>>>                 read_lock(&et->lock);
>>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>>>> index 78c3198a6308..2b5391b229a8 100644
>>>>> --- a/fs/f2fs/segment.c
>>>>> +++ b/fs/f2fs/segment.c
>>>>> @@ -196,9 +196,12 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
>>>>>                 truncate_inode_pages_final(inode->i_mapping);
>>>>>
>>>>>         release_atomic_write_cnt(inode);
>>>>> -     clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
>>>>>         clear_inode_flag(inode, FI_ATOMIC_REPLACE);
>>>>>         clear_inode_flag(inode, FI_ATOMIC_FILE);
>>>>> +     if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) {
>>>>> +             clear_inode_flag(inode, FI_ATOMIC_DIRTIED);
>>>>> +             f2fs_mark_inode_dirty_sync(inode, true);
>>>>> +     }
>>>>>         stat_dec_atomic_inode(inode);
>>>>>
>>>>>         F2FS_I(inode)->atomic_write_task = NULL;
>>>>> @@ -365,7 +368,10 @@ static int __f2fs_commit_atomic_write(struct inode *inode)
>>>>>                 sbi->revoked_atomic_block += fi->atomic_write_cnt;
>>>>>         } else {
>>>>>                 sbi->committed_atomic_block += fi->atomic_write_cnt;
>>>>> -             set_inode_flag(inode, FI_ATOMIC_COMMITTED);
>>>>> +             if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) {
>>>>> +                     clear_inode_flag(inode, FI_ATOMIC_DIRTIED);
>>>>> +                     f2fs_mark_inode_dirty_sync(inode, true);
>>>>> +             }
>>>>>         }
>>>>>
>>>>>         __complete_revoke_list(inode, &revoke_list, ret ? true : false);
>>>>
>>

Re: [f2fs-dev] [PATCH] f2fs: prevent atomic file from being dirtied before commit

[Daeho Jeong]

On Tue, Sep 3, 2024 at 8:35 PM Chao Yu <chao@kernel.org> wrote:
>
> On 2024/9/4 10:52, Daeho Jeong wrote:
> > On Tue, Sep 3, 2024 at 7:26 PM Chao Yu <chao@kernel.org> wrote:
> >>
> >> On 2024/9/4 1:07, Daeho Jeong wrote:
> >>> On Mon, Sep 2, 2024 at 3:08 AM Chao Yu <chao@kernel.org> wrote:
> >>>>
> >>>> On 2024/8/27 4:23, Daeho Jeong wrote:
> >>>>> From: Daeho Jeong <daehojeong@google.com>
> >>>>>
> >>>>> Keep atomic file clean while updating and make it dirtied during commit
> >>>>> in order to avoid unnecessary and excessive inode updates in the previous
> >>>>> fix.
> >>>>>
> >>>>> Fixes: 4bf78322346f ("f2fs: mark inode dirty for FI_ATOMIC_COMMITTED flag")
> >>>>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> >>>>> ---
> >>>>>     fs/f2fs/f2fs.h    |  3 +--
> >>>>>     fs/f2fs/inode.c   | 10 ++++++----
> >>>>>     fs/f2fs/segment.c | 10 ++++++++--
> >>>>>     3 files changed, 15 insertions(+), 8 deletions(-)
> >>>>>
> >>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> >>>>> index 465b2fd50c70..5a7f6fa8b585 100644
> >>>>> --- a/fs/f2fs/f2fs.h
> >>>>> +++ b/fs/f2fs/f2fs.h
> >>>>> @@ -801,7 +801,7 @@ enum {
> >>>>>         FI_COMPRESS_RELEASED,   /* compressed blocks were released */
> >>>>>         FI_ALIGNED_WRITE,       /* enable aligned write */
> >>>>>         FI_COW_FILE,            /* indicate COW file */
> >>>>> -     FI_ATOMIC_COMMITTED,    /* indicate atomic commit completed except disk sync */
> >>>>> +     FI_ATOMIC_DIRTIED,      /* indicate atomic file is dirtied */
> >>>>>         FI_ATOMIC_REPLACE,      /* indicate atomic replace */
> >>>>>         FI_OPENED_FILE,         /* indicate file has been opened */
> >>>>>         FI_MAX,                 /* max flag, never be used */
> >>>>> @@ -3042,7 +3042,6 @@ static inline void __mark_inode_dirty_flag(struct inode *inode,
> >>>>>         case FI_INLINE_DOTS:
> >>>>>         case FI_PIN_FILE:
> >>>>>         case FI_COMPRESS_RELEASED:
> >>>>> -     case FI_ATOMIC_COMMITTED:
> >>>>>                 f2fs_mark_inode_dirty_sync(inode, true);
> >>>>>         }
> >>>>>     }
> >>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> >>>>> index 1eb250c6b392..5dd3e55d2be2 100644
> >>>>> --- a/fs/f2fs/inode.c
> >>>>> +++ b/fs/f2fs/inode.c
> >>>>> @@ -35,6 +35,11 @@ void f2fs_mark_inode_dirty_sync(struct inode *inode, bool sync)
> >>>>>         if (f2fs_inode_dirtied(inode, sync))
> >>>>
> >>>> It will return directly here if inode was dirtied, so it may missed to set
> >>>> FI_ATOMIC_DIRTIED flag?
> >>>
> >>> Is it possible for it to be already dirty, since we already made it
> >>> clean with f2fs_write_inode() when we started the atomic write?
> >>
> >> Some ioctl interfaces may race w/ atomic write? e.g. set_pin_file won't
> >> check atomic_file status, and may dirty inode after we started atomic
> >> write, so we'd better detect such race condition and break ioctl to
> >> avoid ruin atomic write? and maybe we can add f2fs_bug_on() in
> >> f2fs_mark_inode_dirty_sync() to detect any other missing cases?
> >>
> >
> > How about exchanging the positions of f2fs_write_inode() and
> > set_inode_flag() in f2fs_ioc_start_atomic_write()?
> >
> > ...
> >          f2fs_write_inode(inode, NULL);
> >
> >          stat_inc_atomic_inode(inode);
> >
> >          set_inode_flag(inode, FI_ATOMIC_FILE);
> > ...
>
> Oh, I'm not sure I've got your point, after exchanging we still may suffer
> below race condition, right?
>
> - f2fs_ioc_start_atomic_write
>   - set_inode_flag(inode, FI_ATOMIC_FILE)
>   - f2fs_write_inode(inode, NULL)
>                                         - f2fs_ioc_set_pin_file
>                                          - set_inode_flag(inode, FI_PIN_FILE)
>                                           - __mark_inode_dirty_flag()
                                                 => This attempt will
be blocked by the below condition.

+       if (f2fs_is_atomic_file(inode)) {
+               set_inode_flag(inode, FI_ATOMIC_DIRTIED);
+               return;
+       }

Plz, refer to the above comment.

> - f2fs_ioc_commit_atomic_write
>
> So that I proposed a fix for this:
> https://lore.kernel.org/linux-f2fs-devel/20240904032047.1264706-1-chao@kernel.org
>
> Thanks,
>
> >
> >> Thanks,
> >>
> >>>
> >>>>
> >>>> Thanks,
> >>>>
> >>>>>                 return;
> >>>>>
> >>>>> +     if (f2fs_is_atomic_file(inode)) {
> >>>>> +             set_inode_flag(inode, FI_ATOMIC_DIRTIED);
> >>>>> +             return;
> >>>>> +     }
> >>>>> +
> >>>>>         mark_inode_dirty_sync(inode);
> >>>>>     }
> >>>>>
> >>>>> @@ -653,10 +658,7 @@ void f2fs_update_inode(struct inode *inode, struct page *node_page)
> >>>>>         ri->i_gid = cpu_to_le32(i_gid_read(inode));
> >>>>>         ri->i_links = cpu_to_le32(inode->i_nlink);
> >>>>>         ri->i_blocks = cpu_to_le64(SECTOR_TO_BLOCK(inode->i_blocks) + 1);
> >>>>> -
> >>>>> -     if (!f2fs_is_atomic_file(inode) ||
> >>>>> -                     is_inode_flag_set(inode, FI_ATOMIC_COMMITTED))
> >>>>> -             ri->i_size = cpu_to_le64(i_size_read(inode));
> >>>>> +     ri->i_size = cpu_to_le64(i_size_read(inode));
> >>>>>
> >>>>>         if (et) {
> >>>>>                 read_lock(&et->lock);
> >>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> >>>>> index 78c3198a6308..2b5391b229a8 100644
> >>>>> --- a/fs/f2fs/segment.c
> >>>>> +++ b/fs/f2fs/segment.c
> >>>>> @@ -196,9 +196,12 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
> >>>>>                 truncate_inode_pages_final(inode->i_mapping);
> >>>>>
> >>>>>         release_atomic_write_cnt(inode);
> >>>>> -     clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
> >>>>>         clear_inode_flag(inode, FI_ATOMIC_REPLACE);
> >>>>>         clear_inode_flag(inode, FI_ATOMIC_FILE);
> >>>>> +     if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) {
> >>>>> +             clear_inode_flag(inode, FI_ATOMIC_DIRTIED);
> >>>>> +             f2fs_mark_inode_dirty_sync(inode, true);
> >>>>> +     }
> >>>>>         stat_dec_atomic_inode(inode);
> >>>>>
> >>>>>         F2FS_I(inode)->atomic_write_task = NULL;
> >>>>> @@ -365,7 +368,10 @@ static int __f2fs_commit_atomic_write(struct inode *inode)
> >>>>>                 sbi->revoked_atomic_block += fi->atomic_write_cnt;
> >>>>>         } else {
> >>>>>                 sbi->committed_atomic_block += fi->atomic_write_cnt;
> >>>>> -             set_inode_flag(inode, FI_ATOMIC_COMMITTED);
> >>>>> +             if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) {
> >>>>> +                     clear_inode_flag(inode, FI_ATOMIC_DIRTIED);
> >>>>> +                     f2fs_mark_inode_dirty_sync(inode, true);
> >>>>> +             }
> >>>>>         }
> >>>>>
> >>>>>         __complete_revoke_list(inode, &revoke_list, ret ? true : false);
> >>>>
> >>
>

Re: [f2fs-dev] [PATCH] f2fs: prevent atomic file from being dirtied before commit

[Daeho Jeong]

On Wed, Sep 4, 2024 at 7:56 AM Daeho Jeong <daeho43@gmail.com> wrote:
>
> On Tue, Sep 3, 2024 at 8:35 PM Chao Yu <chao@kernel.org> wrote:
> >
> > On 2024/9/4 10:52, Daeho Jeong wrote:
> > > On Tue, Sep 3, 2024 at 7:26 PM Chao Yu <chao@kernel.org> wrote:
> > >>
> > >> On 2024/9/4 1:07, Daeho Jeong wrote:
> > >>> On Mon, Sep 2, 2024 at 3:08 AM Chao Yu <chao@kernel.org> wrote:
> > >>>>
> > >>>> On 2024/8/27 4:23, Daeho Jeong wrote:
> > >>>>> From: Daeho Jeong <daehojeong@google.com>
> > >>>>>
> > >>>>> Keep atomic file clean while updating and make it dirtied during commit
> > >>>>> in order to avoid unnecessary and excessive inode updates in the previous
> > >>>>> fix.
> > >>>>>
> > >>>>> Fixes: 4bf78322346f ("f2fs: mark inode dirty for FI_ATOMIC_COMMITTED flag")
> > >>>>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > >>>>> ---
> > >>>>>     fs/f2fs/f2fs.h    |  3 +--
> > >>>>>     fs/f2fs/inode.c   | 10 ++++++----
> > >>>>>     fs/f2fs/segment.c | 10 ++++++++--
> > >>>>>     3 files changed, 15 insertions(+), 8 deletions(-)
> > >>>>>
> > >>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > >>>>> index 465b2fd50c70..5a7f6fa8b585 100644
> > >>>>> --- a/fs/f2fs/f2fs.h
> > >>>>> +++ b/fs/f2fs/f2fs.h
> > >>>>> @@ -801,7 +801,7 @@ enum {
> > >>>>>         FI_COMPRESS_RELEASED,   /* compressed blocks were released */
> > >>>>>         FI_ALIGNED_WRITE,       /* enable aligned write */
> > >>>>>         FI_COW_FILE,            /* indicate COW file */
> > >>>>> -     FI_ATOMIC_COMMITTED,    /* indicate atomic commit completed except disk sync */
> > >>>>> +     FI_ATOMIC_DIRTIED,      /* indicate atomic file is dirtied */
> > >>>>>         FI_ATOMIC_REPLACE,      /* indicate atomic replace */
> > >>>>>         FI_OPENED_FILE,         /* indicate file has been opened */
> > >>>>>         FI_MAX,                 /* max flag, never be used */
> > >>>>> @@ -3042,7 +3042,6 @@ static inline void __mark_inode_dirty_flag(struct inode *inode,
> > >>>>>         case FI_INLINE_DOTS:
> > >>>>>         case FI_PIN_FILE:
> > >>>>>         case FI_COMPRESS_RELEASED:
> > >>>>> -     case FI_ATOMIC_COMMITTED:
> > >>>>>                 f2fs_mark_inode_dirty_sync(inode, true);
> > >>>>>         }
> > >>>>>     }
> > >>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > >>>>> index 1eb250c6b392..5dd3e55d2be2 100644
> > >>>>> --- a/fs/f2fs/inode.c
> > >>>>> +++ b/fs/f2fs/inode.c
> > >>>>> @@ -35,6 +35,11 @@ void f2fs_mark_inode_dirty_sync(struct inode *inode, bool sync)
> > >>>>>         if (f2fs_inode_dirtied(inode, sync))
> > >>>>
> > >>>> It will return directly here if inode was dirtied, so it may missed to set
> > >>>> FI_ATOMIC_DIRTIED flag?
> > >>>
> > >>> Is it possible for it to be already dirty, since we already made it
> > >>> clean with f2fs_write_inode() when we started the atomic write?
> > >>
> > >> Some ioctl interfaces may race w/ atomic write? e.g. set_pin_file won't
> > >> check atomic_file status, and may dirty inode after we started atomic
> > >> write, so we'd better detect such race condition and break ioctl to
> > >> avoid ruin atomic write? and maybe we can add f2fs_bug_on() in
> > >> f2fs_mark_inode_dirty_sync() to detect any other missing cases?
> > >>
> > >
> > > How about exchanging the positions of f2fs_write_inode() and
> > > set_inode_flag() in f2fs_ioc_start_atomic_write()?
> > >
> > > ...
> > >          f2fs_write_inode(inode, NULL);
> > >
> > >          stat_inc_atomic_inode(inode);
> > >
> > >          set_inode_flag(inode, FI_ATOMIC_FILE);
> > > ...
> >
> > Oh, I'm not sure I've got your point, after exchanging we still may suffer
> > below race condition, right?
> >
> > - f2fs_ioc_start_atomic_write
> >   - set_inode_flag(inode, FI_ATOMIC_FILE)
> >   - f2fs_write_inode(inode, NULL)
> >                                         - f2fs_ioc_set_pin_file
> >                                          - set_inode_flag(inode, FI_PIN_FILE)
> >                                           - __mark_inode_dirty_flag()
>                                                  => This attempt will
> be blocked by the below condition.
>
> +       if (f2fs_is_atomic_file(inode)) {
> +               set_inode_flag(inode, FI_ATOMIC_DIRTIED);
> +               return;
> +       }
>
> Plz, refer to the above comment.

By the way, I need to change this patch a little bit to prevent a
direct inode dirtying case by VFS layer.
Plz, wait for the 2nd one.

>
> > - f2fs_ioc_commit_atomic_write
> >
> > So that I proposed a fix for this:
> > https://lore.kernel.org/linux-f2fs-devel/20240904032047.1264706-1-chao@kernel.org
> >
> > Thanks,
> >
> > >
> > >> Thanks,
> > >>
> > >>>
> > >>>>
> > >>>> Thanks,
> > >>>>
> > >>>>>                 return;
> > >>>>>
> > >>>>> +     if (f2fs_is_atomic_file(inode)) {
> > >>>>> +             set_inode_flag(inode, FI_ATOMIC_DIRTIED);
> > >>>>> +             return;
> > >>>>> +     }
> > >>>>> +
> > >>>>>         mark_inode_dirty_sync(inode);
> > >>>>>     }
> > >>>>>
> > >>>>> @@ -653,10 +658,7 @@ void f2fs_update_inode(struct inode *inode, struct page *node_page)
> > >>>>>         ri->i_gid = cpu_to_le32(i_gid_read(inode));
> > >>>>>         ri->i_links = cpu_to_le32(inode->i_nlink);
> > >>>>>         ri->i_blocks = cpu_to_le64(SECTOR_TO_BLOCK(inode->i_blocks) + 1);
> > >>>>> -
> > >>>>> -     if (!f2fs_is_atomic_file(inode) ||
> > >>>>> -                     is_inode_flag_set(inode, FI_ATOMIC_COMMITTED))
> > >>>>> -             ri->i_size = cpu_to_le64(i_size_read(inode));
> > >>>>> +     ri->i_size = cpu_to_le64(i_size_read(inode));
> > >>>>>
> > >>>>>         if (et) {
> > >>>>>                 read_lock(&et->lock);
> > >>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > >>>>> index 78c3198a6308..2b5391b229a8 100644
> > >>>>> --- a/fs/f2fs/segment.c
> > >>>>> +++ b/fs/f2fs/segment.c
> > >>>>> @@ -196,9 +196,12 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
> > >>>>>                 truncate_inode_pages_final(inode->i_mapping);
> > >>>>>
> > >>>>>         release_atomic_write_cnt(inode);
> > >>>>> -     clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
> > >>>>>         clear_inode_flag(inode, FI_ATOMIC_REPLACE);
> > >>>>>         clear_inode_flag(inode, FI_ATOMIC_FILE);
> > >>>>> +     if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) {
> > >>>>> +             clear_inode_flag(inode, FI_ATOMIC_DIRTIED);
> > >>>>> +             f2fs_mark_inode_dirty_sync(inode, true);
> > >>>>> +     }
> > >>>>>         stat_dec_atomic_inode(inode);
> > >>>>>
> > >>>>>         F2FS_I(inode)->atomic_write_task = NULL;
> > >>>>> @@ -365,7 +368,10 @@ static int __f2fs_commit_atomic_write(struct inode *inode)
> > >>>>>                 sbi->revoked_atomic_block += fi->atomic_write_cnt;
> > >>>>>         } else {
> > >>>>>                 sbi->committed_atomic_block += fi->atomic_write_cnt;
> > >>>>> -             set_inode_flag(inode, FI_ATOMIC_COMMITTED);
> > >>>>> +             if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) {
> > >>>>> +                     clear_inode_flag(inode, FI_ATOMIC_DIRTIED);
> > >>>>> +                     f2fs_mark_inode_dirty_sync(inode, true);
> > >>>>> +             }
> > >>>>>         }
> > >>>>>
> > >>>>>         __complete_revoke_list(inode, &revoke_list, ret ? true : false);
> > >>>>
> > >>
> >

Re: [f2fs-dev] [PATCH] f2fs: prevent atomic file from being dirtied before commit

[Chao Yu]

On 2024/9/4 22:56, Daeho Jeong wrote:
> On Tue, Sep 3, 2024 at 8:35 PM Chao Yu <chao@kernel.org> wrote:
>>
>> On 2024/9/4 10:52, Daeho Jeong wrote:
>>> On Tue, Sep 3, 2024 at 7:26 PM Chao Yu <chao@kernel.org> wrote:
>>>>
>>>> On 2024/9/4 1:07, Daeho Jeong wrote:
>>>>> On Mon, Sep 2, 2024 at 3:08 AM Chao Yu <chao@kernel.org> wrote:
>>>>>>
>>>>>> On 2024/8/27 4:23, Daeho Jeong wrote:
>>>>>>> From: Daeho Jeong <daehojeong@google.com>
>>>>>>>
>>>>>>> Keep atomic file clean while updating and make it dirtied during commit
>>>>>>> in order to avoid unnecessary and excessive inode updates in the previous
>>>>>>> fix.
>>>>>>>
>>>>>>> Fixes: 4bf78322346f ("f2fs: mark inode dirty for FI_ATOMIC_COMMITTED flag")
>>>>>>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
>>>>>>> ---
>>>>>>>      fs/f2fs/f2fs.h    |  3 +--
>>>>>>>      fs/f2fs/inode.c   | 10 ++++++----
>>>>>>>      fs/f2fs/segment.c | 10 ++++++++--
>>>>>>>      3 files changed, 15 insertions(+), 8 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>>>>> index 465b2fd50c70..5a7f6fa8b585 100644
>>>>>>> --- a/fs/f2fs/f2fs.h
>>>>>>> +++ b/fs/f2fs/f2fs.h
>>>>>>> @@ -801,7 +801,7 @@ enum {
>>>>>>>          FI_COMPRESS_RELEASED,   /* compressed blocks were released */
>>>>>>>          FI_ALIGNED_WRITE,       /* enable aligned write */
>>>>>>>          FI_COW_FILE,            /* indicate COW file */
>>>>>>> -     FI_ATOMIC_COMMITTED,    /* indicate atomic commit completed except disk sync */
>>>>>>> +     FI_ATOMIC_DIRTIED,      /* indicate atomic file is dirtied */
>>>>>>>          FI_ATOMIC_REPLACE,      /* indicate atomic replace */
>>>>>>>          FI_OPENED_FILE,         /* indicate file has been opened */
>>>>>>>          FI_MAX,                 /* max flag, never be used */
>>>>>>> @@ -3042,7 +3042,6 @@ static inline void __mark_inode_dirty_flag(struct inode *inode,
>>>>>>>          case FI_INLINE_DOTS:
>>>>>>>          case FI_PIN_FILE:
>>>>>>>          case FI_COMPRESS_RELEASED:
>>>>>>> -     case FI_ATOMIC_COMMITTED:
>>>>>>>                  f2fs_mark_inode_dirty_sync(inode, true);
>>>>>>>          }
>>>>>>>      }
>>>>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>>>>>> index 1eb250c6b392..5dd3e55d2be2 100644
>>>>>>> --- a/fs/f2fs/inode.c
>>>>>>> +++ b/fs/f2fs/inode.c
>>>>>>> @@ -35,6 +35,11 @@ void f2fs_mark_inode_dirty_sync(struct inode *inode, bool sync)
>>>>>>>          if (f2fs_inode_dirtied(inode, sync))
>>>>>>
>>>>>> It will return directly here if inode was dirtied, so it may missed to set
>>>>>> FI_ATOMIC_DIRTIED flag?
>>>>>
>>>>> Is it possible for it to be already dirty, since we already made it
>>>>> clean with f2fs_write_inode() when we started the atomic write?
>>>>
>>>> Some ioctl interfaces may race w/ atomic write? e.g. set_pin_file won't
>>>> check atomic_file status, and may dirty inode after we started atomic
>>>> write, so we'd better detect such race condition and break ioctl to
>>>> avoid ruin atomic write? and maybe we can add f2fs_bug_on() in
>>>> f2fs_mark_inode_dirty_sync() to detect any other missing cases?
>>>>
>>>
>>> How about exchanging the positions of f2fs_write_inode() and
>>> set_inode_flag() in f2fs_ioc_start_atomic_write()?
>>>
>>> ...
>>>           f2fs_write_inode(inode, NULL);
>>>
>>>           stat_inc_atomic_inode(inode);
>>>
>>>           set_inode_flag(inode, FI_ATOMIC_FILE);
>>> ...
>>
>> Oh, I'm not sure I've got your point, after exchanging we still may suffer
>> below race condition, right?
>>
>> - f2fs_ioc_start_atomic_write
>>    - set_inode_flag(inode, FI_ATOMIC_FILE)
>>    - f2fs_write_inode(inode, NULL)
>>                                          - f2fs_ioc_set_pin_file
>>                                           - set_inode_flag(inode, FI_PIN_FILE)
>>                                            - __mark_inode_dirty_flag()
>                                                   => This attempt will
> be blocked by the below condition.
> 
> +       if (f2fs_is_atomic_file(inode)) {
> +               set_inode_flag(inode, FI_ATOMIC_DIRTIED);
> +               return;
> +       }

Oh, yes, FI_ATOMIC_DIRTIED will be tagged once inode becomes dirty.

Thanks,

> 
> Plz, refer to the above comment.
> 
>> - f2fs_ioc_commit_atomic_write
>>
>> So that I proposed a fix for this:
>> https://lore.kernel.org/linux-f2fs-devel/20240904032047.1264706-1-chao@kernel.org
>>
>> Thanks,
>>
>>>
>>>> Thanks,
>>>>
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>>>                  return;
>>>>>>>
>>>>>>> +     if (f2fs_is_atomic_file(inode)) {
>>>>>>> +             set_inode_flag(inode, FI_ATOMIC_DIRTIED);
>>>>>>> +             return;
>>>>>>> +     }
>>>>>>> +
>>>>>>>          mark_inode_dirty_sync(inode);
>>>>>>>      }
>>>>>>>
>>>>>>> @@ -653,10 +658,7 @@ void f2fs_update_inode(struct inode *inode, struct page *node_page)
>>>>>>>          ri->i_gid = cpu_to_le32(i_gid_read(inode));
>>>>>>>          ri->i_links = cpu_to_le32(inode->i_nlink);
>>>>>>>          ri->i_blocks = cpu_to_le64(SECTOR_TO_BLOCK(inode->i_blocks) + 1);
>>>>>>> -
>>>>>>> -     if (!f2fs_is_atomic_file(inode) ||
>>>>>>> -                     is_inode_flag_set(inode, FI_ATOMIC_COMMITTED))
>>>>>>> -             ri->i_size = cpu_to_le64(i_size_read(inode));
>>>>>>> +     ri->i_size = cpu_to_le64(i_size_read(inode));
>>>>>>>
>>>>>>>          if (et) {
>>>>>>>                  read_lock(&et->lock);
>>>>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>>>>>> index 78c3198a6308..2b5391b229a8 100644
>>>>>>> --- a/fs/f2fs/segment.c
>>>>>>> +++ b/fs/f2fs/segment.c
>>>>>>> @@ -196,9 +196,12 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
>>>>>>>                  truncate_inode_pages_final(inode->i_mapping);
>>>>>>>
>>>>>>>          release_atomic_write_cnt(inode);
>>>>>>> -     clear_inode_flag(inode, FI_ATOMIC_COMMITTED);
>>>>>>>          clear_inode_flag(inode, FI_ATOMIC_REPLACE);
>>>>>>>          clear_inode_flag(inode, FI_ATOMIC_FILE);
>>>>>>> +     if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) {
>>>>>>> +             clear_inode_flag(inode, FI_ATOMIC_DIRTIED);
>>>>>>> +             f2fs_mark_inode_dirty_sync(inode, true);
>>>>>>> +     }
>>>>>>>          stat_dec_atomic_inode(inode);
>>>>>>>
>>>>>>>          F2FS_I(inode)->atomic_write_task = NULL;
>>>>>>> @@ -365,7 +368,10 @@ static int __f2fs_commit_atomic_write(struct inode *inode)
>>>>>>>                  sbi->revoked_atomic_block += fi->atomic_write_cnt;
>>>>>>>          } else {
>>>>>>>                  sbi->committed_atomic_block += fi->atomic_write_cnt;
>>>>>>> -             set_inode_flag(inode, FI_ATOMIC_COMMITTED);
>>>>>>> +             if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) {
>>>>>>> +                     clear_inode_flag(inode, FI_ATOMIC_DIRTIED);
>>>>>>> +                     f2fs_mark_inode_dirty_sync(inode, true);
>>>>>>> +             }
>>>>>>>          }
>>>>>>>
>>>>>>>          __complete_revoke_list(inode, &revoke_list, ret ? true : false);
>>>>>>
>>>>
>>