* [PATCH v2] f2fs: introduce F2FS_IOC_START_ATOMIC_REPLACE
@ 2022-09-20 1:40 Daeho Jeong
2022-09-29 7:36 ` [f2fs-dev] " Chao Yu
0 siblings, 1 reply; 10+ messages in thread
From: Daeho Jeong @ 2022-09-20 1:40 UTC (permalink / raw)
To: linux-kernel, linux-f2fs-devel, kernel-team; +Cc: 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>
---
v2: add undefined ioctl number reported by <lkp@intel.com>
---
fs/f2fs/data.c | 3 +++
fs/f2fs/f2fs.h | 1 +
fs/f2fs/file.c | 12 ++++++++++--
fs/f2fs/segment.c | 14 +++++++++++++-
include/uapi/linux/f2fs.h | 1 +
5 files changed, 28 insertions(+), 3 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 4f9b80c41b1e..4abd9d2a55b3 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,6 +2051,12 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
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);
spin_lock(&sbi->inode_lock[ATOMIC_FILE]);
@@ -4080,7 +4086,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 143b7ea0fb8e..c524538a9013 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -263,14 +263,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.37.3.968.ga6b4b080e4-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [f2fs-dev] [PATCH v2] f2fs: introduce F2FS_IOC_START_ATOMIC_REPLACE
2022-09-20 1:40 [PATCH v2] f2fs: introduce F2FS_IOC_START_ATOMIC_REPLACE Daeho Jeong
@ 2022-09-29 7:36 ` Chao Yu
2022-09-29 16:13 ` Daeho Jeong
0 siblings, 1 reply; 10+ messages in thread
From: Chao Yu @ 2022-09-29 7:36 UTC (permalink / raw)
To: Daeho Jeong, linux-kernel, linux-f2fs-devel, kernel-team; +Cc: Daeho Jeong
On 2022/9/20 9:40, 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.
>
> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> ---
> v2: add undefined ioctl number reported by <lkp@intel.com>
> ---
> fs/f2fs/data.c | 3 +++
> fs/f2fs/f2fs.h | 1 +
> fs/f2fs/file.c | 12 ++++++++++--
> fs/f2fs/segment.c | 14 +++++++++++++-
> include/uapi/linux/f2fs.h | 1 +
> 5 files changed, 28 insertions(+), 3 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 4f9b80c41b1e..4abd9d2a55b3 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,6 +2051,12 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
>
> 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;
> + }
Hi Daeho,
isize should be updated after tagging inode as atomic_write one?
otherwise f2fs_mark_inode_dirty_sync() may update isize to inode page,
latter checkpoint may persist that change? IIUC...
Thanks,
> f2fs_i_size_write(fi->cow_inode, isize);
>
> spin_lock(&sbi->inode_lock[ATOMIC_FILE]);
> @@ -4080,7 +4086,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 143b7ea0fb8e..c524538a9013 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -263,14 +263,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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [f2fs-dev] [PATCH v2] f2fs: introduce F2FS_IOC_START_ATOMIC_REPLACE
2022-09-29 7:36 ` [f2fs-dev] " Chao Yu
@ 2022-09-29 16:13 ` Daeho Jeong
2022-09-29 22:54 ` Chao Yu
0 siblings, 1 reply; 10+ messages in thread
From: Daeho Jeong @ 2022-09-29 16:13 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong
On Thu, Sep 29, 2022 at 12:36 AM Chao Yu <chao@kernel.org> wrote:
>
> On 2022/9/20 9:40, 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.
> >
> > Signed-off-by: Daeho Jeong <daehojeong@google.com>
> > ---
> > v2: add undefined ioctl number reported by <lkp@intel.com>
> > ---
> > fs/f2fs/data.c | 3 +++
> > fs/f2fs/f2fs.h | 1 +
> > fs/f2fs/file.c | 12 ++++++++++--
> > fs/f2fs/segment.c | 14 +++++++++++++-
> > include/uapi/linux/f2fs.h | 1 +
> > 5 files changed, 28 insertions(+), 3 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 4f9b80c41b1e..4abd9d2a55b3 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,6 +2051,12 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
> >
> > 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;
> > + }
>
> Hi Daeho,
>
> isize should be updated after tagging inode as atomic_write one?
> otherwise f2fs_mark_inode_dirty_sync() may update isize to inode page,
> latter checkpoint may persist that change? IIUC...
>
> Thanks,
Hi Chao,
The first patch of this patchset prevents the inode page from being
updated as dirty for atomic file cases.
Is there any other chances for the inode page to be marked as dirty?
Thanks,
>
> > f2fs_i_size_write(fi->cow_inode, isize);
> >
> > spin_lock(&sbi->inode_lock[ATOMIC_FILE]);
> > @@ -4080,7 +4086,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 143b7ea0fb8e..c524538a9013 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -263,14 +263,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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [f2fs-dev] [PATCH v2] f2fs: introduce F2FS_IOC_START_ATOMIC_REPLACE
2022-09-29 16:13 ` Daeho Jeong
@ 2022-09-29 22:54 ` Chao Yu
2022-09-29 23:33 ` Daeho Jeong
0 siblings, 1 reply; 10+ messages in thread
From: Chao Yu @ 2022-09-29 22:54 UTC (permalink / raw)
To: Daeho Jeong; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong
On 2022/9/30 0:13, Daeho Jeong wrote:
> On Thu, Sep 29, 2022 at 12:36 AM Chao Yu <chao@kernel.org> wrote:
>>
>> On 2022/9/20 9:40, 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.
>>>
>>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
>>> ---
>>> v2: add undefined ioctl number reported by <lkp@intel.com>
>>> ---
>>> fs/f2fs/data.c | 3 +++
>>> fs/f2fs/f2fs.h | 1 +
>>> fs/f2fs/file.c | 12 ++++++++++--
>>> fs/f2fs/segment.c | 14 +++++++++++++-
>>> include/uapi/linux/f2fs.h | 1 +
>>> 5 files changed, 28 insertions(+), 3 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 4f9b80c41b1e..4abd9d2a55b3 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,6 +2051,12 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
>>>
>>> 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;
>>> + }
>>
>> Hi Daeho,
>>
>> isize should be updated after tagging inode as atomic_write one?
>> otherwise f2fs_mark_inode_dirty_sync() may update isize to inode page,
>> latter checkpoint may persist that change? IIUC...
>>
>> Thanks,
>
> Hi Chao,
>
> The first patch of this patchset prevents the inode page from being
> updated as dirty for atomic file cases.
> Is there any other chances for the inode page to be marked as dirty?
I mean:
Thread A Thread B
- f2fs_ioc_start_atomic_write
- f2fs_i_size_write(inode, 0)
- f2fs_mark_inode_dirty_sync
- checkpoint
- persist inode with incorrect zero isize
- set_inode_flag(inode, FI_ATOMIC_FILE)
Am I missing something?
Thanks,
>
> Thanks,
>
>>
>>> f2fs_i_size_write(fi->cow_inode, isize);
>>>
>>> spin_lock(&sbi->inode_lock[ATOMIC_FILE]);
>>> @@ -4080,7 +4086,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 143b7ea0fb8e..c524538a9013 100644
>>> --- a/fs/f2fs/segment.c
>>> +++ b/fs/f2fs/segment.c
>>> @@ -263,14 +263,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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [f2fs-dev] [PATCH v2] f2fs: introduce F2FS_IOC_START_ATOMIC_REPLACE
2022-09-29 22:54 ` Chao Yu
@ 2022-09-29 23:33 ` Daeho Jeong
2022-09-30 0:49 ` Chao Yu
0 siblings, 1 reply; 10+ messages in thread
From: Daeho Jeong @ 2022-09-29 23:33 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong
On Thu, Sep 29, 2022 at 3:54 PM Chao Yu <chao@kernel.org> wrote:
>
> On 2022/9/30 0:13, Daeho Jeong wrote:
> > On Thu, Sep 29, 2022 at 12:36 AM Chao Yu <chao@kernel.org> wrote:
> >>
> >> On 2022/9/20 9:40, 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.
> >>>
> >>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> >>> ---
> >>> v2: add undefined ioctl number reported by <lkp@intel.com>
> >>> ---
> >>> fs/f2fs/data.c | 3 +++
> >>> fs/f2fs/f2fs.h | 1 +
> >>> fs/f2fs/file.c | 12 ++++++++++--
> >>> fs/f2fs/segment.c | 14 +++++++++++++-
> >>> include/uapi/linux/f2fs.h | 1 +
> >>> 5 files changed, 28 insertions(+), 3 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 4f9b80c41b1e..4abd9d2a55b3 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,6 +2051,12 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
> >>>
> >>> 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;
> >>> + }
> >>
> >> Hi Daeho,
> >>
> >> isize should be updated after tagging inode as atomic_write one?
> >> otherwise f2fs_mark_inode_dirty_sync() may update isize to inode page,
> >> latter checkpoint may persist that change? IIUC...
> >>
> >> Thanks,
> >
> > Hi Chao,
> >
> > The first patch of this patchset prevents the inode page from being
> > updated as dirty for atomic file cases.
> > Is there any other chances for the inode page to be marked as dirty?
>
> I mean:
>
> Thread A Thread B
> - f2fs_ioc_start_atomic_write
> - f2fs_i_size_write(inode, 0)
> - f2fs_mark_inode_dirty_sync
> - checkpoint
> - persist inode with incorrect zero isize
>
> - set_inode_flag(inode, FI_ATOMIC_FILE)
>
> Am I missing something?
>
So, f2fs_mark_inode_dirty_sync() will not work for atomic files
anymore, which means it doesn't make the inode dirty.
Plz, refer to the first patch of this patchset. Or I might be confused
with something. :(
@@ -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);
}
> Thanks,
>
> >
> > Thanks,
> >
> >>
> >>> f2fs_i_size_write(fi->cow_inode, isize);
> >>>
> >>> spin_lock(&sbi->inode_lock[ATOMIC_FILE]);
> >>> @@ -4080,7 +4086,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 143b7ea0fb8e..c524538a9013 100644
> >>> --- a/fs/f2fs/segment.c
> >>> +++ b/fs/f2fs/segment.c
> >>> @@ -263,14 +263,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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [f2fs-dev] [PATCH v2] f2fs: introduce F2FS_IOC_START_ATOMIC_REPLACE
2022-09-29 23:33 ` Daeho Jeong
@ 2022-09-30 0:49 ` Chao Yu
2022-09-30 16:04 ` Daeho Jeong
0 siblings, 1 reply; 10+ messages in thread
From: Chao Yu @ 2022-09-30 0:49 UTC (permalink / raw)
To: Daeho Jeong; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong
On 2022/9/30 7:33, Daeho Jeong wrote:
> On Thu, Sep 29, 2022 at 3:54 PM Chao Yu <chao@kernel.org> wrote:
>>
>> On 2022/9/30 0:13, Daeho Jeong wrote:
>>> On Thu, Sep 29, 2022 at 12:36 AM Chao Yu <chao@kernel.org> wrote:
>>>>
>>>> On 2022/9/20 9:40, 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.
>>>>>
>>>>> Signed-off-by: Daeho Jeong <daehojeong@google.com>
>>>>> ---
>>>>> v2: add undefined ioctl number reported by <lkp@intel.com>
>>>>> ---
>>>>> fs/f2fs/data.c | 3 +++
>>>>> fs/f2fs/f2fs.h | 1 +
>>>>> fs/f2fs/file.c | 12 ++++++++++--
>>>>> fs/f2fs/segment.c | 14 +++++++++++++-
>>>>> include/uapi/linux/f2fs.h | 1 +
>>>>> 5 files changed, 28 insertions(+), 3 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 4f9b80c41b1e..4abd9d2a55b3 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,6 +2051,12 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
>>>>>
>>>>> 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;
>>>>> + }
>>>>
>>>> Hi Daeho,
>>>>
>>>> isize should be updated after tagging inode as atomic_write one?
>>>> otherwise f2fs_mark_inode_dirty_sync() may update isize to inode page,
>>>> latter checkpoint may persist that change? IIUC...
>>>>
>>>> Thanks,
>>>
>>> Hi Chao,
>>>
>>> The first patch of this patchset prevents the inode page from being
>>> updated as dirty for atomic file cases.
>>> Is there any other chances for the inode page to be marked as dirty?
>>
>> I mean:
>>
>> Thread A Thread B
>> - f2fs_ioc_start_atomic_write
>> - f2fs_i_size_write(inode, 0)
>> - f2fs_mark_inode_dirty_sync
>> - checkpoint
>> - persist inode with incorrect zero isize
>>
>> - set_inode_flag(inode, FI_ATOMIC_FILE)
>>
>> Am I missing something?
>>
>
> So, f2fs_mark_inode_dirty_sync() will not work for atomic files
> anymore, which means it doesn't make the inode dirty.
> Plz, refer to the first patch of this patchset. Or I might be confused
> with something. :(
I mean FI_ATOMIC_FILE was set after f2fs_i_size_write(), so inode will be set
as dirty.
Thanks,
>
> @@ -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);
> }
>
>
>
>
>
>> Thanks,
>>
>>>
>>> Thanks,
>>>
>>>>
>>>>> f2fs_i_size_write(fi->cow_inode, isize);
>>>>>
>>>>> spin_lock(&sbi->inode_lock[ATOMIC_FILE]);
>>>>> @@ -4080,7 +4086,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 143b7ea0fb8e..c524538a9013 100644
>>>>> --- a/fs/f2fs/segment.c
>>>>> +++ b/fs/f2fs/segment.c
>>>>> @@ -263,14 +263,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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [f2fs-dev] [PATCH v2] f2fs: introduce F2FS_IOC_START_ATOMIC_REPLACE
2022-09-30 0:49 ` Chao Yu
@ 2022-09-30 16:04 ` Daeho Jeong
2022-09-30 20:01 ` Daeho Jeong
0 siblings, 1 reply; 10+ messages in thread
From: Daeho Jeong @ 2022-09-30 16:04 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong
> >>>>
> >>>> Hi Daeho,
> >>>>
> >>>> isize should be updated after tagging inode as atomic_write one?
> >>>> otherwise f2fs_mark_inode_dirty_sync() may update isize to inode page,
> >>>> latter checkpoint may persist that change? IIUC...
> >>>>
> >>>> Thanks,
> >>>
> >>> Hi Chao,
> >>>
> >>> The first patch of this patchset prevents the inode page from being
> >>> updated as dirty for atomic file cases.
> >>> Is there any other chances for the inode page to be marked as dirty?
> >>
> >> I mean:
> >>
> >> Thread A Thread B
> >> - f2fs_ioc_start_atomic_write
> >> - f2fs_i_size_write(inode, 0)
> >> - f2fs_mark_inode_dirty_sync
> >> - checkpoint
> >> - persist inode with incorrect zero isize
> >>
> >> - set_inode_flag(inode, FI_ATOMIC_FILE)
> >>
> >> Am I missing something?
> >>
> >
> > So, f2fs_mark_inode_dirty_sync() will not work for atomic files
> > anymore, which means it doesn't make the inode dirty.
> > Plz, refer to the first patch of this patchset. Or I might be confused
> > with something. :(
>
> I mean FI_ATOMIC_FILE was set after f2fs_i_size_write(), so inode will be set
> as dirty.
>
> Thanks,
>
Oh, I was confused that f2fs_update_inode() is called in
f2fs_mark_inode_dirty_sync().
That is called in f2fs_write_inode(). Let me fix this.
Thanks,
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [f2fs-dev] [PATCH v2] f2fs: introduce F2FS_IOC_START_ATOMIC_REPLACE
2022-09-30 16:04 ` Daeho Jeong
@ 2022-09-30 20:01 ` Daeho Jeong
2022-10-01 0:16 ` Chao Yu
0 siblings, 1 reply; 10+ messages in thread
From: Daeho Jeong @ 2022-09-30 20:01 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong
On Fri, Sep 30, 2022 at 9:04 AM Daeho Jeong <daeho43@gmail.com> wrote:
>
> > >>>>
> > >>>> Hi Daeho,
> > >>>>
> > >>>> isize should be updated after tagging inode as atomic_write one?
> > >>>> otherwise f2fs_mark_inode_dirty_sync() may update isize to inode page,
> > >>>> latter checkpoint may persist that change? IIUC...
> > >>>>
> > >>>> Thanks,
> > >>>
> > >>> Hi Chao,
> > >>>
> > >>> The first patch of this patchset prevents the inode page from being
> > >>> updated as dirty for atomic file cases.
> > >>> Is there any other chances for the inode page to be marked as dirty?
> > >>
> > >> I mean:
> > >>
> > >> Thread A Thread B
> > >> - f2fs_ioc_start_atomic_write
> > >> - f2fs_i_size_write(inode, 0)
> > >> - f2fs_mark_inode_dirty_sync
> > >> - checkpoint
> > >> - persist inode with incorrect zero isize
> > >>
> > >> - set_inode_flag(inode, FI_ATOMIC_FILE)
> > >>
> > >> Am I missing something?
> > >>
> > >
> > > So, f2fs_mark_inode_dirty_sync() will not work for atomic files
> > > anymore, which means it doesn't make the inode dirty.
> > > Plz, refer to the first patch of this patchset. Or I might be confused
> > > with something. :(
> >
> > I mean FI_ATOMIC_FILE was set after f2fs_i_size_write(), so inode will be set
> > as dirty.
> >
> > Thanks,
> >
>
> Oh, I was confused that f2fs_update_inode() is called in
> f2fs_mark_inode_dirty_sync().
> That is called in f2fs_write_inode(). Let me fix this.
Hmm, I think the issue was already there before my patch.
So, how about making the inode flushed and clean if the inode is
already dirty when starting atomic write?
>
> Thanks,
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [f2fs-dev] [PATCH v2] f2fs: introduce F2FS_IOC_START_ATOMIC_REPLACE
2022-09-30 20:01 ` Daeho Jeong
@ 2022-10-01 0:16 ` Chao Yu
2022-10-03 15:59 ` Daeho Jeong
0 siblings, 1 reply; 10+ messages in thread
From: Chao Yu @ 2022-10-01 0:16 UTC (permalink / raw)
To: Daeho Jeong; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong
On 2022/10/1 4:01, Daeho Jeong wrote:
> On Fri, Sep 30, 2022 at 9:04 AM Daeho Jeong <daeho43@gmail.com> wrote:
>>
>>>>>>>
>>>>>>> Hi Daeho,
>>>>>>>
>>>>>>> isize should be updated after tagging inode as atomic_write one?
>>>>>>> otherwise f2fs_mark_inode_dirty_sync() may update isize to inode page,
>>>>>>> latter checkpoint may persist that change? IIUC...
>>>>>>>
>>>>>>> Thanks,
>>>>>>
>>>>>> Hi Chao,
>>>>>>
>>>>>> The first patch of this patchset prevents the inode page from being
>>>>>> updated as dirty for atomic file cases.
>>>>>> Is there any other chances for the inode page to be marked as dirty?
>>>>>
>>>>> I mean:
>>>>>
>>>>> Thread A Thread B
>>>>> - f2fs_ioc_start_atomic_write
>>>>> - f2fs_i_size_write(inode, 0)
>>>>> - f2fs_mark_inode_dirty_sync
>>>>> - checkpoint
>>>>> - persist inode with incorrect zero isize
>>>>>
>>>>> - set_inode_flag(inode, FI_ATOMIC_FILE)
>>>>>
>>>>> Am I missing something?
>>>>>
>>>>
>>>> So, f2fs_mark_inode_dirty_sync() will not work for atomic files
>>>> anymore, which means it doesn't make the inode dirty.
>>>> Plz, refer to the first patch of this patchset. Or I might be confused
>>>> with something. :(
>>>
>>> I mean FI_ATOMIC_FILE was set after f2fs_i_size_write(), so inode will be set
>>> as dirty.
>>>
>>> Thanks,
>>>
>>
>> Oh, I was confused that f2fs_update_inode() is called in
>> f2fs_mark_inode_dirty_sync().
>> That is called in f2fs_write_inode(). Let me fix this.
>
> Hmm, I think the issue was already there before my patch.
> So, how about making the inode flushed and clean if the inode is
> already dirty when starting atomic write?
What I mean is:
---
fs/f2fs/file.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index e4b6e51086a3..31b229678b1d 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2053,6 +2053,9 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
isize = i_size_read(inode);
fi->original_i_size = isize;
+
+ set_inode_flag(inode, FI_ATOMIC_FILE);
+
if (truncate) {
set_inode_flag(inode, FI_ATOMIC_REPLACE);
truncate_inode_pages_final(inode->i_mapping);
@@ -2063,7 +2066,6 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
stat_inc_atomic_inode(inode);
- set_inode_flag(inode, FI_ATOMIC_FILE);
set_inode_flag(fi->cow_inode, FI_COW_FILE);
clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
--
Let's set FI_ATOMIC_FILE flag before f2fs_i_size_write(inode, 0), so
- f2fs_ioc_start_atomic_write
- f2fs_i_size_write(, 0)
- f2fs_mark_inode_dirty_sync
check f2fs_is_atomic_file() and return correctly.
And for the case the inode is dirty before f2fs_i_size_write(, 0), we
can call f2fs_write_inode() to flush dirty feilds into inode page, and
make inode clean.
>
>>
>> Thanks,
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [f2fs-dev] [PATCH v2] f2fs: introduce F2FS_IOC_START_ATOMIC_REPLACE
2022-10-01 0:16 ` Chao Yu
@ 2022-10-03 15:59 ` Daeho Jeong
0 siblings, 0 replies; 10+ messages in thread
From: Daeho Jeong @ 2022-10-03 15:59 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong
> What I mean is:
>
> ---
> fs/f2fs/file.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index e4b6e51086a3..31b229678b1d 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -2053,6 +2053,9 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
>
> isize = i_size_read(inode);
> fi->original_i_size = isize;
> +
> + set_inode_flag(inode, FI_ATOMIC_FILE);
> +
> if (truncate) {
> set_inode_flag(inode, FI_ATOMIC_REPLACE);
> truncate_inode_pages_final(inode->i_mapping);
> @@ -2063,7 +2066,6 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate)
>
> stat_inc_atomic_inode(inode);
>
> - set_inode_flag(inode, FI_ATOMIC_FILE);
> set_inode_flag(fi->cow_inode, FI_COW_FILE);
> clear_inode_flag(fi->cow_inode, FI_INLINE_DATA);
> f2fs_up_write(&fi->i_gc_rwsem[WRITE]);
> --
>
>
> Let's set FI_ATOMIC_FILE flag before f2fs_i_size_write(inode, 0), so
> - f2fs_ioc_start_atomic_write
> - f2fs_i_size_write(, 0)
> - f2fs_mark_inode_dirty_sync
> check f2fs_is_atomic_file() and return correctly.
>
Ah, I got it.
> And for the case the inode is dirty before f2fs_i_size_write(, 0), we
> can call f2fs_write_inode() to flush dirty feilds into inode page, and
> make inode clean.
>
Thanks for the tips~
> >
> >>
> >> Thanks,
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-10-03 15:59 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-20 1:40 [PATCH v2] f2fs: introduce F2FS_IOC_START_ATOMIC_REPLACE Daeho Jeong
2022-09-29 7:36 ` [f2fs-dev] " Chao Yu
2022-09-29 16:13 ` Daeho Jeong
2022-09-29 22:54 ` Chao Yu
2022-09-29 23:33 ` Daeho Jeong
2022-09-30 0:49 ` Chao Yu
2022-09-30 16:04 ` Daeho Jeong
2022-09-30 20:01 ` Daeho Jeong
2022-10-01 0:16 ` Chao Yu
2022-10-03 15:59 ` Daeho Jeong
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).