[PATCH v4 1/2] f2fs: correct i_size change for atomic writes

[PATCH v4 1/2] f2fs: correct i_size change for atomic writes

[Daeho Jeong]

From: Daeho Jeong <daehojeong@google.com>

We need to make sure i_size doesn't change until atomic write commit is
successful and restore it when commit is failed.

Signed-off-by: Daeho Jeong <daehojeong@google.com>
---
v4: move i_size update after clearing atomic file flag in
    f2fs_abort_atomic_write()
v3: make sure inode is clean while atomic writing
---
 fs/f2fs/f2fs.h    |  1 +
 fs/f2fs/file.c    | 18 +++++++++++-------
 fs/f2fs/inode.c   |  3 +++
 fs/f2fs/segment.c |  7 +++++--
 4 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index dee7b67a17a6..539da7f12cfc 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -821,6 +821,7 @@ struct f2fs_inode_info {
 	unsigned int i_cluster_size;		/* cluster size */
 
 	unsigned int atomic_write_cnt;
+	loff_t original_i_size;		/* original i_size before atomic write */
 };
 
 static inline void get_extent_info(struct extent_info *ext,
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 5efe0e4a725a..ce2336d2f688 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1989,6 +1989,7 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
 	struct f2fs_inode_info *fi = F2FS_I(inode);
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 	struct inode *pinode;
+	loff_t isize;
 	int ret;
 
 	if (!inode_owner_or_capable(mnt_userns, inode))
@@ -2047,7 +2048,12 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
 		f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
 		goto out;
 	}
-	f2fs_i_size_write(fi->cow_inode, i_size_read(inode));
+
+	f2fs_write_inode(inode, NULL);
+
+	isize = i_size_read(inode);
+	fi->original_i_size = isize;
+	f2fs_i_size_write(fi->cow_inode, isize);
 
 	spin_lock(&sbi->inode_lock[ATOMIC_FILE]);
 	sbi->atomic_files++;
@@ -2087,16 +2093,14 @@ static int f2fs_ioc_commit_atomic_write(struct file *filp)
 
 	if (f2fs_is_atomic_file(inode)) {
 		ret = f2fs_commit_atomic_write(inode);
-		if (ret)
-			goto unlock_out;
-
-		ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 0, true);
 		if (!ret)
-			f2fs_abort_atomic_write(inode, false);
+			ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 0, true);
+
+		f2fs_abort_atomic_write(inode, ret);
 	} else {
 		ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 1, false);
 	}
-unlock_out:
+
 	inode_unlock(inode);
 	mnt_drop_write_file(filp);
 	return ret;
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index cde0a3dc80c3..64d7772b4cd9 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -30,6 +30,9 @@ void f2fs_mark_inode_dirty_sync(struct inode *inode, bool sync)
 	if (f2fs_inode_dirtied(inode, sync))
 		return;
 
+	if (f2fs_is_atomic_file(inode))
+		return;
+
 	mark_inode_dirty_sync(inode);
 }
 
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 460048f3c850..abb55cd418c1 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -193,14 +193,17 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
 	if (!f2fs_is_atomic_file(inode))
 		return;
 
-	if (clean)
-		truncate_inode_pages_final(inode->i_mapping);
 	clear_inode_flag(fi->cow_inode, FI_COW_FILE);
 	iput(fi->cow_inode);
 	fi->cow_inode = NULL;
 	release_atomic_write_cnt(inode);
 	clear_inode_flag(inode, FI_ATOMIC_FILE);
 
+	if (clean) {
+		truncate_inode_pages_final(inode->i_mapping);
+		f2fs_i_size_write(inode, fi->original_i_size);
+	}
+
 	spin_lock(&sbi->inode_lock[ATOMIC_FILE]);
 	sbi->atomic_files--;
 	spin_unlock(&sbi->inode_lock[ATOMIC_FILE]);
-- 
2.38.0.rc1.362.ged0d419d3c-goog

[PATCH v4 2/2] f2fs: introduce F2FS_IOC_START_ATOMIC_REPLACE

[Daeho Jeong]

From: Daeho Jeong <daehojeong@google.com>

introduce a new ioctl to replace the whole content of a file atomically,
which means it induces truncate and content update at the same time.
We can start it with F2FS_IOC_START_ATOMIC_REPLACE and complete it with
F2FS_IOC_COMMIT_ATOMIC_WRITE. Or abort it with
F2FS_IOC_ABORT_ATOMIC_WRITE.

Signed-off-by: Daeho Jeong <daehojeong@google.com>
---
v3: move i_size change after setting atomic write flag
v2: add undefined ioctl number reported by <lkp@intel.com>
---
 fs/f2fs/data.c            |  3 +++
 fs/f2fs/f2fs.h            |  1 +
 fs/f2fs/file.c            | 20 ++++++++++++++------
 fs/f2fs/segment.c         | 14 +++++++++++++-
 include/uapi/linux/f2fs.h |  1 +
 5 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 6cd29a575105..d3d32db3a25d 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -3438,6 +3438,9 @@ static int prepare_atomic_write_begin(struct f2fs_sb_info *sbi,
 	else if (*blk_addr != NULL_ADDR)
 		return 0;
 
+	if (is_inode_flag_set(inode, FI_ATOMIC_REPLACE))
+		goto reserve_block;
+
 	/* Look for the block in the original inode */
 	err = __find_data_block(inode, index, &ori_blk_addr);
 	if (err)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 539da7f12cfc..2c49da12d6d8 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -764,6 +764,7 @@ enum {
 	FI_COMPRESS_RELEASED,	/* compressed blocks were released */
 	FI_ALIGNED_WRITE,	/* enable aligned write */
 	FI_COW_FILE,		/* indicate COW file */
+	FI_ATOMIC_REPLACE,	/* indicate atomic replace */
 	FI_MAX,			/* max flag, never be used */
 };
 
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index ce2336d2f688..66d62ea42c73 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1982,7 +1982,7 @@ static int f2fs_ioc_getversion(struct file *filp, unsigned long arg)
 	return put_user(inode->i_generation, (int __user *)arg);
 }
 
-static int f2fs_ioc_start_atomic_write(struct file *filp)
+static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
 {
 	struct inode *inode = file_inode(filp);
 	struct user_namespace *mnt_userns = file_mnt_user_ns(filp);
@@ -2051,10 +2051,6 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
 
 	f2fs_write_inode(inode, NULL);
 
-	isize = i_size_read(inode);
-	fi->original_i_size = isize;
-	f2fs_i_size_write(fi->cow_inode, isize);
-
 	spin_lock(&sbi->inode_lock[ATOMIC_FILE]);
 	sbi->atomic_files++;
 	spin_unlock(&sbi->inode_lock[ATOMIC_FILE]);
@@ -2064,6 +2060,16 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
 	clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
 	f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
 
+	isize = i_size_read(inode);
+	fi->original_i_size = isize;
+	if (truncate) {
+		set_inode_flag(inode, FI_ATOMIC_REPLACE);
+		truncate_inode_pages_final(inode->i_mapping);
+		f2fs_i_size_write(inode, 0);
+		isize = 0;
+	}
+	f2fs_i_size_write(fi->cow_inode, isize);
+
 	f2fs_update_time(sbi, REQ_TIME);
 	fi->atomic_write_task = current;
 	stat_update_max_atomic_write(inode);
@@ -4082,7 +4088,9 @@ static long __f2fs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	case FS_IOC_GETVERSION:
 		return f2fs_ioc_getversion(filp, arg);
 	case F2FS_IOC_START_ATOMIC_WRITE:
-		return f2fs_ioc_start_atomic_write(filp);
+		return f2fs_ioc_start_atomic_write(filp, false);
+	case F2FS_IOC_START_ATOMIC_REPLACE:
+		return f2fs_ioc_start_atomic_write(filp, true);
 	case F2FS_IOC_COMMIT_ATOMIC_WRITE:
 		return f2fs_ioc_commit_atomic_write(filp);
 	case F2FS_IOC_ABORT_ATOMIC_WRITE:
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index abb55cd418c1..e4f7efbdc027 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -264,14 +264,26 @@ static void __complete_revoke_list(struct inode *inode, struct list_head *head,
 					bool revoke)
 {
 	struct revoke_entry *cur, *tmp;
+	pgoff_t start_index = 0;
+	bool truncate = is_inode_flag_set(inode, FI_ATOMIC_REPLACE);
 
 	list_for_each_entry_safe(cur, tmp, head, list) {
-		if (revoke)
+		if (revoke) {
 			__replace_atomic_write_block(inode, cur->index,
 						cur->old_addr, NULL, true);
+		} else if (truncate) {
+			f2fs_truncate_hole(inode, start_index, cur->index);
+			start_index = cur->index + 1;
+		}
+
 		list_del(&cur->list);
 		kmem_cache_free(revoke_entry_slab, cur);
 	}
+
+	if (!revoke && truncate) {
+		f2fs_do_truncate_blocks(inode, start_index * PAGE_SIZE, false);
+		clear_inode_flag(inode, FI_ATOMIC_REPLACE);
+	}
 }
 
 static int __f2fs_commit_atomic_write(struct inode *inode)
diff --git a/include/uapi/linux/f2fs.h b/include/uapi/linux/f2fs.h
index 3121d127d5aa..955d440be104 100644
--- a/include/uapi/linux/f2fs.h
+++ b/include/uapi/linux/f2fs.h
@@ -42,6 +42,7 @@
 						struct f2fs_comp_option)
 #define F2FS_IOC_DECOMPRESS_FILE	_IO(F2FS_IOCTL_MAGIC, 23)
 #define F2FS_IOC_COMPRESS_FILE		_IO(F2FS_IOCTL_MAGIC, 24)
+#define F2FS_IOC_START_ATOMIC_REPLACE	_IO(F2FS_IOCTL_MAGIC, 25)
 
 /*
  * should be same as XFS_IOC_GOINGDOWN.
-- 
2.38.0.rc1.362.ged0d419d3c-goog

Re: [f2fs-dev] [PATCH v4 1/2] f2fs: correct i_size change for atomic writes

[Chao Yu]

On 2022/10/5 1:13, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@google.com>
> 
> We need to make sure i_size doesn't change until atomic write commit is
> successful and restore it when commit is failed.
> 
> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> ---
> v4: move i_size update after clearing atomic file flag in
>      f2fs_abort_atomic_write()
> v3: make sure inode is clean while atomic writing
> ---
>   fs/f2fs/f2fs.h    |  1 +
>   fs/f2fs/file.c    | 18 +++++++++++-------
>   fs/f2fs/inode.c   |  3 +++
>   fs/f2fs/segment.c |  7 +++++--
>   4 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index dee7b67a17a6..539da7f12cfc 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -821,6 +821,7 @@ struct f2fs_inode_info {
>   	unsigned int i_cluster_size;		/* cluster size */
>   
>   	unsigned int atomic_write_cnt;
> +	loff_t original_i_size;		/* original i_size before atomic write */
>   };
>   
>   static inline void get_extent_info(struct extent_info *ext,
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 5efe0e4a725a..ce2336d2f688 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -1989,6 +1989,7 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
>   	struct f2fs_inode_info *fi = F2FS_I(inode);
>   	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>   	struct inode *pinode;
> +	loff_t isize;
>   	int ret;
>   
>   	if (!inode_owner_or_capable(mnt_userns, inode))
> @@ -2047,7 +2048,12 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
>   		f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
>   		goto out;
>   	}
> -	f2fs_i_size_write(fi->cow_inode, i_size_read(inode));
> +
> +	f2fs_write_inode(inode, NULL);
> +
> +	isize = i_size_read(inode);
> +	fi->original_i_size = isize;
> +	f2fs_i_size_write(fi->cow_inode, isize);
>   
>   	spin_lock(&sbi->inode_lock[ATOMIC_FILE]);
>   	sbi->atomic_files++;
> @@ -2087,16 +2093,14 @@ static int f2fs_ioc_commit_atomic_write(struct file *filp)
>   
>   	if (f2fs_is_atomic_file(inode)) {
>   		ret = f2fs_commit_atomic_write(inode);
> -		if (ret)
> -			goto unlock_out;
> -
> -		ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 0, true);
>   		if (!ret)
> -			f2fs_abort_atomic_write(inode, false);
> +			ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 0, true);
> +
> +		f2fs_abort_atomic_write(inode, ret);
>   	} else {
>   		ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 1, false);
>   	}
> -unlock_out:
> +
>   	inode_unlock(inode);
>   	mnt_drop_write_file(filp);
>   	return ret;
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index cde0a3dc80c3..64d7772b4cd9 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -30,6 +30,9 @@ void f2fs_mark_inode_dirty_sync(struct inode *inode, bool sync)
>   	if (f2fs_inode_dirtied(inode, sync))
>   		return;
>   
> +	if (f2fs_is_atomic_file(inode))
> +		return;

One question, after f2fs_inode_dirtied(), atomic_file is added to
inode_list[DIRTY_META], and it will be flushed by checkpoint()
triggered in between write() and atomic_commit ioctl, is it not
expected that inode w/ new i_size will be persisted?

Should write_end() skip updating atomic_file's i_size and let
atomic_commit() update it if there is no error?

Thanks,

> +
>   	mark_inode_dirty_sync(inode);
>   }
>   
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 460048f3c850..abb55cd418c1 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -193,14 +193,17 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
>   	if (!f2fs_is_atomic_file(inode))
>   		return;
>   
> -	if (clean)
> -		truncate_inode_pages_final(inode->i_mapping);
>   	clear_inode_flag(fi->cow_inode, FI_COW_FILE);
>   	iput(fi->cow_inode);
>   	fi->cow_inode = NULL;
>   	release_atomic_write_cnt(inode);
>   	clear_inode_flag(inode, FI_ATOMIC_FILE);
>   
> +	if (clean) {
> +		truncate_inode_pages_final(inode->i_mapping);
> +		f2fs_i_size_write(inode, fi->original_i_size);
> +	}
> +
>   	spin_lock(&sbi->inode_lock[ATOMIC_FILE]);
>   	sbi->atomic_files--;
>   	spin_unlock(&sbi->inode_lock[ATOMIC_FILE]);

Re: [f2fs-dev] [PATCH v4 1/2] f2fs: correct i_size change for atomic writes

[Daeho Jeong]

On Wed, Oct 5, 2022 at 6:46 AM Chao Yu <chao@kernel.org> wrote:
>
> On 2022/10/5 1:13, Daeho Jeong wrote:
> > From: Daeho Jeong <daehojeong@google.com>
> >
> > We need to make sure i_size doesn't change until atomic write commit is
> > successful and restore it when commit is failed.
> >
> > Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > ---
> > v4: move i_size update after clearing atomic file flag in
> >      f2fs_abort_atomic_write()
> > v3: make sure inode is clean while atomic writing
> > ---
> >   fs/f2fs/f2fs.h    |  1 +
> >   fs/f2fs/file.c    | 18 +++++++++++-------
> >   fs/f2fs/inode.c   |  3 +++
> >   fs/f2fs/segment.c |  7 +++++--
> >   4 files changed, 20 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index dee7b67a17a6..539da7f12cfc 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -821,6 +821,7 @@ struct f2fs_inode_info {
> >       unsigned int i_cluster_size;            /* cluster size */
> >
> >       unsigned int atomic_write_cnt;
> > +     loff_t original_i_size;         /* original i_size before atomic write */
> >   };
> >
> >   static inline void get_extent_info(struct extent_info *ext,
> > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > index 5efe0e4a725a..ce2336d2f688 100644
> > --- a/fs/f2fs/file.c
> > +++ b/fs/f2fs/file.c
> > @@ -1989,6 +1989,7 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
> >       struct f2fs_inode_info *fi = F2FS_I(inode);
> >       struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> >       struct inode *pinode;
> > +     loff_t isize;
> >       int ret;
> >
> >       if (!inode_owner_or_capable(mnt_userns, inode))
> > @@ -2047,7 +2048,12 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
> >               f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> >               goto out;
> >       }
> > -     f2fs_i_size_write(fi->cow_inode, i_size_read(inode));
> > +
> > +     f2fs_write_inode(inode, NULL);
> > +
> > +     isize = i_size_read(inode);
> > +     fi->original_i_size = isize;
> > +     f2fs_i_size_write(fi->cow_inode, isize);
> >
> >       spin_lock(&sbi->inode_lock[ATOMIC_FILE]);
> >       sbi->atomic_files++;
> > @@ -2087,16 +2093,14 @@ static int f2fs_ioc_commit_atomic_write(struct file *filp)
> >
> >       if (f2fs_is_atomic_file(inode)) {
> >               ret = f2fs_commit_atomic_write(inode);
> > -             if (ret)
> > -                     goto unlock_out;
> > -
> > -             ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 0, true);
> >               if (!ret)
> > -                     f2fs_abort_atomic_write(inode, false);
> > +                     ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 0, true);
> > +
> > +             f2fs_abort_atomic_write(inode, ret);
> >       } else {
> >               ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 1, false);
> >       }
> > -unlock_out:
> > +
> >       inode_unlock(inode);
> >       mnt_drop_write_file(filp);
> >       return ret;
> > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > index cde0a3dc80c3..64d7772b4cd9 100644
> > --- a/fs/f2fs/inode.c
> > +++ b/fs/f2fs/inode.c
> > @@ -30,6 +30,9 @@ void f2fs_mark_inode_dirty_sync(struct inode *inode, bool sync)
> >       if (f2fs_inode_dirtied(inode, sync))
> >               return;
> >
> > +     if (f2fs_is_atomic_file(inode))
> > +             return;
>
> One question, after f2fs_inode_dirtied(), atomic_file is added to
> inode_list[DIRTY_META], and it will be flushed by checkpoint()
> triggered in between write() and atomic_commit ioctl, is it not
> expected that inode w/ new i_size will be persisted?

Isn't it okay if we move f2fs_is_atomic_file() ahead of f2fs_inode_dirtied()?

>
> Should write_end() skip updating atomic_file's i_size and let
> atomic_commit() update it if there is no error?

In this case, the user can't see the changed i_size while writing an
atomic file.

>
> Thanks,
>
> > +
> >       mark_inode_dirty_sync(inode);
> >   }
> >
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index 460048f3c850..abb55cd418c1 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -193,14 +193,17 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
> >       if (!f2fs_is_atomic_file(inode))
> >               return;
> >
> > -     if (clean)
> > -             truncate_inode_pages_final(inode->i_mapping);
> >       clear_inode_flag(fi->cow_inode, FI_COW_FILE);
> >       iput(fi->cow_inode);
> >       fi->cow_inode = NULL;
> >       release_atomic_write_cnt(inode);
> >       clear_inode_flag(inode, FI_ATOMIC_FILE);
> >
> > +     if (clean) {
> > +             truncate_inode_pages_final(inode->i_mapping);
> > +             f2fs_i_size_write(inode, fi->original_i_size);
> > +     }
> > +
> >       spin_lock(&sbi->inode_lock[ATOMIC_FILE]);
> >       sbi->atomic_files--;
> >       spin_unlock(&sbi->inode_lock[ATOMIC_FILE]);

Re: [PATCH v4 2/2] f2fs: introduce F2FS_IOC_START_ATOMIC_REPLACE

[Christoph Hellwig]

On Tue, Oct 04, 2022 at 10:13:51AM -0700, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@google.com>
> 
> introduce a new ioctl to replace the whole content of a file atomically,
> which means it induces truncate and content update at the same time.
> We can start it with F2FS_IOC_START_ATOMIC_REPLACE and complete it with
> F2FS_IOC_COMMIT_ATOMIC_WRITE. Or abort it with
> F2FS_IOC_ABORT_ATOMIC_WRITE.

It would be great to Cc Darrick and linux-fsdevel as there have been
attempts to do this properly at the VFS level instead of a completely
undocumented ioctl.

Re: [PATCH v4 2/2] f2fs: introduce F2FS_IOC_START_ATOMIC_REPLACE

[Darrick J. Wong]

On Thu, Oct 06, 2022 at 01:33:34AM -0700, Christoph Hellwig wrote:
> On Tue, Oct 04, 2022 at 10:13:51AM -0700, Daeho Jeong wrote:
> > From: Daeho Jeong <daehojeong@google.com>
> > 
> > introduce a new ioctl to replace the whole content of a file atomically,
> > which means it induces truncate and content update at the same time.
> > We can start it with F2FS_IOC_START_ATOMIC_REPLACE and complete it with
> > F2FS_IOC_COMMIT_ATOMIC_WRITE. Or abort it with
> > F2FS_IOC_ABORT_ATOMIC_WRITE.
> 
> It would be great to Cc Darrick and linux-fsdevel as there have been
> attempts to do this properly at the VFS level instead of a completely
> undocumented ioctl.

It's been a while since I sent the last RFC, but yes, it's still in my
queue as part of the xfs online fsck patchserieses.

https://lore.kernel.org/linux-fsdevel/161723932606.3149451.12366114306150243052.stgit@magnolia/

More recent git branch:
https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=atomic-file-updates

--D

Re: [f2fs-dev] [PATCH v4 1/2] f2fs: correct i_size change for atomic writes

[Chao Yu]

On 2022/10/6 0:06, Daeho Jeong wrote:
> On Wed, Oct 5, 2022 at 6:46 AM Chao Yu <chao@kernel.org> wrote:
>>
>> On 2022/10/5 1:13, Daeho Jeong wrote:
>>> From: Daeho Jeong <daehojeong@google.com>
>>>
>>> We need to make sure i_size doesn't change until atomic write commit is
>>> successful and restore it when commit is failed.
>>>
>>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
>>> ---
>>> v4: move i_size update after clearing atomic file flag in
>>>       f2fs_abort_atomic_write()
>>> v3: make sure inode is clean while atomic writing
>>> ---
>>>    fs/f2fs/f2fs.h    |  1 +
>>>    fs/f2fs/file.c    | 18 +++++++++++-------
>>>    fs/f2fs/inode.c   |  3 +++
>>>    fs/f2fs/segment.c |  7 +++++--
>>>    4 files changed, 20 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>> index dee7b67a17a6..539da7f12cfc 100644
>>> --- a/fs/f2fs/f2fs.h
>>> +++ b/fs/f2fs/f2fs.h
>>> @@ -821,6 +821,7 @@ struct f2fs_inode_info {
>>>        unsigned int i_cluster_size;            /* cluster size */
>>>
>>>        unsigned int atomic_write_cnt;
>>> +     loff_t original_i_size;         /* original i_size before atomic write */
>>>    };
>>>
>>>    static inline void get_extent_info(struct extent_info *ext,
>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>> index 5efe0e4a725a..ce2336d2f688 100644
>>> --- a/fs/f2fs/file.c
>>> +++ b/fs/f2fs/file.c
>>> @@ -1989,6 +1989,7 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
>>>        struct f2fs_inode_info *fi = F2FS_I(inode);
>>>        struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>>>        struct inode *pinode;
>>> +     loff_t isize;
>>>        int ret;
>>>
>>>        if (!inode_owner_or_capable(mnt_userns, inode))
>>> @@ -2047,7 +2048,12 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
>>>                f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
>>>                goto out;
>>>        }
>>> -     f2fs_i_size_write(fi->cow_inode, i_size_read(inode));
>>> +
>>> +     f2fs_write_inode(inode, NULL);
>>> +
>>> +     isize = i_size_read(inode);
>>> +     fi->original_i_size = isize;
>>> +     f2fs_i_size_write(fi->cow_inode, isize);
>>>
>>>        spin_lock(&sbi->inode_lock[ATOMIC_FILE]);
>>>        sbi->atomic_files++;
>>> @@ -2087,16 +2093,14 @@ static int f2fs_ioc_commit_atomic_write(struct file *filp)
>>>
>>>        if (f2fs_is_atomic_file(inode)) {
>>>                ret = f2fs_commit_atomic_write(inode);
>>> -             if (ret)
>>> -                     goto unlock_out;
>>> -
>>> -             ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 0, true);
>>>                if (!ret)
>>> -                     f2fs_abort_atomic_write(inode, false);
>>> +                     ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 0, true);
>>> +
>>> +             f2fs_abort_atomic_write(inode, ret);
>>>        } else {
>>>                ret = f2fs_do_sync_file(filp, 0, LLONG_MAX, 1, false);
>>>        }
>>> -unlock_out:
>>> +
>>>        inode_unlock(inode);
>>>        mnt_drop_write_file(filp);
>>>        return ret;
>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>> index cde0a3dc80c3..64d7772b4cd9 100644
>>> --- a/fs/f2fs/inode.c
>>> +++ b/fs/f2fs/inode.c
>>> @@ -30,6 +30,9 @@ void f2fs_mark_inode_dirty_sync(struct inode *inode, bool sync)
>>>        if (f2fs_inode_dirtied(inode, sync))
>>>                return;
>>>
>>> +     if (f2fs_is_atomic_file(inode))
>>> +             return;
>>
>> One question, after f2fs_inode_dirtied(), atomic_file is added to
>> inode_list[DIRTY_META], and it will be flushed by checkpoint()
>> triggered in between write() and atomic_commit ioctl, is it not
>> expected that inode w/ new i_size will be persisted?
> 
> Isn't it okay if we move f2fs_is_atomic_file() ahead of f2fs_inode_dirtied()?

Fine to me.

But another question is, now it allows GC to migrate blocks belong
to atomic files, so, during migration, it may update extent cache,
once largest extent was updated, it will mark inode dirty, but after
this patch, it may lose the extent change? thoughts?

> 
>>
>> Should write_end() skip updating atomic_file's i_size and let
>> atomic_commit() update it if there is no error?
> 
> In this case, the user can't see the changed i_size while writing an
> atomic file.

Oh, right, please ignore this comment...

Thanks,

> 
>>
>> Thanks,
>>
>>> +
>>>        mark_inode_dirty_sync(inode);
>>>    }
>>>
>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>> index 460048f3c850..abb55cd418c1 100644
>>> --- a/fs/f2fs/segment.c
>>> +++ b/fs/f2fs/segment.c
>>> @@ -193,14 +193,17 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean)
>>>        if (!f2fs_is_atomic_file(inode))
>>>                return;
>>>
>>> -     if (clean)
>>> -             truncate_inode_pages_final(inode->i_mapping);
>>>        clear_inode_flag(fi->cow_inode, FI_COW_FILE);
>>>        iput(fi->cow_inode);
>>>        fi->cow_inode = NULL;
>>>        release_atomic_write_cnt(inode);
>>>        clear_inode_flag(inode, FI_ATOMIC_FILE);
>>>
>>> +     if (clean) {
>>> +             truncate_inode_pages_final(inode->i_mapping);
>>> +             f2fs_i_size_write(inode, fi->original_i_size);
>>> +     }
>>> +
>>>        spin_lock(&sbi->inode_lock[ATOMIC_FILE]);
>>>        sbi->atomic_files--;
>>>        spin_unlock(&sbi->inode_lock[ATOMIC_FILE]);

Re: [f2fs-dev] [PATCH v4 1/2] f2fs: correct i_size change for atomic writes

[Daeho Jeong]

>
> Fine to me.
>
> But another question is, now it allows GC to migrate blocks belong
> to atomic files, so, during migration, it may update extent cache,
> once largest extent was updated, it will mark inode dirty, but after
> this patch, it may lose the extent change? thoughts?
>

Oh, I missed that case. Maybe we could prevent updating the i_size of
atomic files in f2fs_update_inode() while allowing inode dirtying.

Re: [f2fs-dev] [PATCH v4 1/2] f2fs: correct i_size change for atomic writes

[Chao Yu]

On 2022/10/7 22:22, Daeho Jeong wrote:
>>
>> Fine to me.
>>
>> But another question is, now it allows GC to migrate blocks belong
>> to atomic files, so, during migration, it may update extent cache,
>> once largest extent was updated, it will mark inode dirty, but after
>> this patch, it may lose the extent change? thoughts?
>>
> 
> Oh, I missed that case. Maybe we could prevent updating the i_size of
> atomic files in f2fs_update_inode() while allowing inode dirtying.

Agreed. :)

Thanks,

Re: [PATCH v4 2/2] f2fs: introduce F2FS_IOC_START_ATOMIC_REPLACE

[Daeho Jeong]

On Thu, Oct 6, 2022 at 9:32 AM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Thu, Oct 06, 2022 at 01:33:34AM -0700, Christoph Hellwig wrote:
> > On Tue, Oct 04, 2022 at 10:13:51AM -0700, Daeho Jeong wrote:
> > > From: Daeho Jeong <daehojeong@google.com>
> > >
> > > introduce a new ioctl to replace the whole content of a file atomically,
> > > which means it induces truncate and content update at the same time.
> > > We can start it with F2FS_IOC_START_ATOMIC_REPLACE and complete it with
> > > F2FS_IOC_COMMIT_ATOMIC_WRITE. Or abort it with
> > > F2FS_IOC_ABORT_ATOMIC_WRITE.
> >
> > It would be great to Cc Darrick and linux-fsdevel as there have been
> > attempts to do this properly at the VFS level instead of a completely
> > undocumented ioctl.
>
> It's been a while since I sent the last RFC, but yes, it's still in my
> queue as part of the xfs online fsck patchserieses.
>
> https://lore.kernel.org/linux-fsdevel/161723932606.3149451.12366114306150243052.stgit@magnolia/
>
> More recent git branch:
> https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=atomic-file-updates

Hi,

It's a very interesting suggestion and we might use this in F2FS someday.
However, I think it's not exactly matched for what
F2FS_IOC_START_ATOMIC_REPLACE is doing now.

Thanks for bringing my attention to this.

>
> --D