Re: [f2fs-dev] [PATCH] f2fs: don't return vmalloc() memory from f2fs_kmalloc()
On 2020/5/7 2:43, Eric Biggers wrote: > On Wed, May 06, 2020 at 03:43:36PM +0800, Chao Yu wrote: >> On 2020/5/6 4:48, Eric Biggers wrote: >>> From: Eric Biggers >>> >>> kmalloc() returns kmalloc'ed memory, and kvmalloc() returns either >>> kmalloc'ed or vmalloc'ed memory. But the f2fs wrappers, f2fs_kmalloc() >>> and f2fs_kvmalloc(), both return both kinds of memory. >>> >>> It's redundant to have two functions that do the same thing, and also >>> breaking the standard naming convention is causing bugs since people >>> assume it's safe to kfree() memory allocated by f2fs_kmalloc(). See >>> e.g. the various allocations in fs/f2fs/compress.c. >>> >>> Fix this by making f2fs_kmalloc() just use kmalloc(). And to avoid >>> re-introducing the allocation failures that the vmalloc fallback was >>> intended to fix, convert the largest allocations to use f2fs_kvmalloc(). >> >> I've submitted one patch since you suggested when commented in compression >> support patch. >> >> I remember Jaegeuk prefer to use f2fs_kvmalloc() to instead f2fs_kmalloc(), >> and keep the order of kmalloc - failed - kvmalloc. >> > > I think you're talking about > https://lkml.kernel.org/r/20191105030451.ga55...@jaegeuk-macbookpro.roam.corp.google.com? > > I think that making the large allocations use kvmalloc(), as this patch does, > is > good enough to address any memory allocation failures that may have been > encountered in the past. We don't need to switch all allocations to use > kvmalloc(), as there's no benefit for small allocations. Yeah, I agreed, and in ENOMEM case, small-sized kvmalloc increases allocating path length unnecessarily. Thanks, > > - Eric > . > ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v2] f2fs: shrink spinlock coverage
On 2020/5/6 23:05, Jaegeuk Kim wrote:
> On 05/06, Chao Yu wrote:
>> In f2fs_try_to_free_nids(), .nid_list_lock spinlock critical region will
>> increase as expected shrink number increase, to avoid spining other CPUs
>> for long time, it's better to implement like extent cache and nats
>> shrinker.
>>
>> Signed-off-by: Chao Yu
>> ---
>> v2:
>> - fix unlock wrong spinlock.
>> fs/f2fs/node.c | 15 +++
>> 1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>> index 4da0d8713df5..ad0b14f4dab8 100644
>> --- a/fs/f2fs/node.c
>> +++ b/fs/f2fs/node.c
>> @@ -2488,7 +2488,6 @@ void f2fs_alloc_nid_failed(struct f2fs_sb_info *sbi,
>> nid_t nid)
>> int f2fs_try_to_free_nids(struct f2fs_sb_info *sbi, int nr_shrink)
>> {
>> struct f2fs_nm_info *nm_i = NM_I(sbi);
>> -struct free_nid *i, *next;
>> int nr = nr_shrink;
>>
>> if (nm_i->nid_cnt[FREE_NID] <= MAX_FREE_NIDS)
>> @@ -2498,14 +2497,22 @@ int f2fs_try_to_free_nids(struct f2fs_sb_info *sbi,
>> int nr_shrink)
>> return 0;
>>
>> spin_lock(_i->nid_list_lock);
>> -list_for_each_entry_safe(i, next, _i->free_nid_list, list) {
>> -if (nr_shrink <= 0 ||
>> -nm_i->nid_cnt[FREE_NID] <= MAX_FREE_NIDS)
>> +while (nr_shrink) {
>> +struct free_nid *i;
>> +
>> +if (nm_i->nid_cnt[FREE_NID] <= MAX_FREE_NIDS)
>> break;
>>
>> +i = list_first_entry(_i->free_nid_list,
>> +struct free_nid, list);
>> +list_del(>list);
>> +spin_unlock(_i->nid_list_lock);
>> +
>> __remove_free_nid(sbi, i, FREE_NID);
>
> __remove_free_nid() will do list_del again. btw, how about just splitting out
Oh, my bad.
How about moving __remove_free_nid into .nid_list_lock coverage?
> given nr_shrink into multiple trials?
Like this?
while (shrink) {
batch = DEFAULT_BATCH_NUMBER; // 16
spinlock();
list_for_each_entry_safe() {
if (!shrink || !batch)
break;
remove_item_from_list;
shrink--;
batch--;
}
spin_unlock();
}
Thanks,
>
>> kmem_cache_free(free_nid_slab, i);
>> nr_shrink--;
>> +
>> +spin_lock(_i->nid_list_lock);
>> }
>> spin_unlock(_i->nid_list_lock);
>> mutex_unlock(_i->build_lock);
>> --
>> 2.18.0.rc1
> .
>
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: change maximum zstd compression buffer size
On 2020/5/6 22:56, Jaegeuk Kim wrote: > On 05/06, Chao Yu wrote: >> On 2020/5/6 7:05, Jaegeuk Kim wrote: >>> On 05/05, Chao Yu wrote: On 2020-5-4 22:30, Jaegeuk Kim wrote: > From: Daeho Jeong > > Current zstd compression buffer size is one page and header size less > than cluster size. By this, zstd compression always succeeds even if > the real compression data is failed to fit into the buffer size, and > eventually reading the cluster returns I/O error with the corrupted > compression data. What's the root cause of this issue? I didn't get it. > > Signed-off-by: Daeho Jeong > --- > fs/f2fs/compress.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c > index 4c7eaeee52336..a9fa8049b295f 100644 > --- a/fs/f2fs/compress.c > +++ b/fs/f2fs/compress.c > @@ -313,7 +313,7 @@ static int zstd_init_compress_ctx(struct compress_ctx > *cc) > cc->private = workspace; > cc->private2 = stream; > > - cc->clen = cc->rlen - PAGE_SIZE - COMPRESS_HEADER_SIZE; > + cc->clen = ZSTD_compressBound(PAGE_SIZE << cc->log_cluster_size); In my machine, the value is 66572 which is much larger than size of dst buffer, so, in where we can tell the real size of dst buffer to zstd compressor? Otherwise, if compressed data size is larger than dst buffer size, when we flush compressed data into dst buffer, we may suffer overflow. >>> >>> Could you give it a try compress_log_size=2 and check decompression works? >> >> I tried some samples before submitting the patch, did you encounter app's >> data >> corruption when using zstd algorithm? If so, let me check this issue. > > Daeho reported: > 1. cp -a src_file comp_dir/dst_file (comp_dir is a directory for compression) > 2. sync comp_dir/dst_file > 3. echo 3 > /proc/sys/vm/drop_caches > 4. cat comp_dir/dst_file > dump (it returns I/O error depending on the file). Let me check this issue. Thanks, > >> >> Thanks, >> >>> > return 0; > } > > @@ -330,7 +330,7 @@ static int zstd_compress_pages(struct compress_ctx > *cc) > ZSTD_inBuffer inbuf; > ZSTD_outBuffer outbuf; > int src_size = cc->rlen; > - int dst_size = src_size - PAGE_SIZE - COMPRESS_HEADER_SIZE; > + int dst_size = cc->clen; > int ret; > > inbuf.pos = 0; > >>> >>> >>> ___ >>> Linux-f2fs-devel mailing list >>> Linux-f2fs-devel@lists.sourceforge.net >>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel >>> . >>> > . > ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 1/3] f2fs_io: add fsync
On 2020/5/6 22:39, Jaegeuk Kim wrote:
> On 05/06, Chao Yu wrote:
>> On 2020/5/2 8:29, Jaegeuk Kim wrote:
>>> Signed-off-by: Jaegeuk Kim
>>> ---
>>> tools/f2fs_io/f2fs_io.c | 25 +
>>> 1 file changed, 25 insertions(+)
>>>
>>> diff --git a/tools/f2fs_io/f2fs_io.c b/tools/f2fs_io/f2fs_io.c
>>> index c1edef1..c84b6ab 100644
>>> --- a/tools/f2fs_io/f2fs_io.c
>>> +++ b/tools/f2fs_io/f2fs_io.c
>>> @@ -130,6 +130,30 @@ static void full_write(int fd, const void *buf, size_t
>>> count)
>>> }
>>> }
>>>
>>> +#define fsync_desc "fsync"
>>> +#define fsync_help \
>>> +"f2fs_io fsync [file]\n\n" \
>>
>> What about supporting fdatasync via an additional argument here?
>
> I prefer to add another command "fdatasync" for simplicity. :P
LGTM as well. :)
Thanks
>
>>
>>> +"fsync given the file\n" \
>>> +
>>> +static void do_fsync(int argc, char **argv, const struct cmd_desc *cmd)
>>> +{
>>> + int fd;
>>> +
>>> + if (argc != 2) {
>>> + fputs("Excess arguments\n\n", stderr);
>>> + fputs(cmd->cmd_help, stderr);
>>> + exit(1);
>>> + }
>>> +
>>> + fd = xopen(argv[1], O_WRONLY, 0);
>>> +
>>> + if (fsync(fd) != 0)
>>> + die_errno("fsync failed");
>>> +
>>> + printf("fsync a file\n");
>>> + exit(0);
>>> +}
>>> +
>>> #define set_verity_desc "Set fs-verity"
>>> #define set_verity_help\
>>> "f2fs_io set_verity [file]\n\n"\
>>> @@ -780,6 +804,7 @@ static void do_reserve_cblocks(int argc, char **argv,
>>> const struct cmd_desc *cmd
>>> static void do_help(int argc, char **argv, const struct cmd_desc *cmd);
>>> const struct cmd_desc cmd_list[] = {
>>> _CMD(help),
>>> + CMD(fsync),
>>> CMD(set_verity),
>>> CMD(getflags),
>>> CMD(setflags),
>>>
> .
>
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: get parent inode when recovering pino
On Wed, May 06, 2020 at 12:16:13PM -0700, Eric Biggers wrote: > On Wed, May 06, 2020 at 02:47:19PM +0800, Gao Xiang wrote: > > On Wed, May 06, 2020 at 09:58:22AM +0800, Gao Xiang wrote: > > > On Tue, May 05, 2020 at 06:24:28PM -0700, Eric Biggers wrote: > > > > On Wed, May 06, 2020 at 08:14:07AM +0800, Gao Xiang wrote: > > > > > > > > > > > > Actually, I think this is wrong because the fsync can be done via a > > > > > > file > > > > > > descriptor that was opened to a now-deleted link to the file. > > > > > > > > > > I'm still confused about this... > > > > > > > > > > I don't know what's wrong with this version from my limited knowledge? > > > > > inode itself is locked when fsyncing, so > > > > > > > > > >if the fsync inode->i_nlink == 1, this inode has only one hard link > > > > >(not deleted yet) and should belong to a single directory; and > > > > > > > > > >the only one parent directory would not go away (not deleted as > > > > > well) > > > > >since there are some dirents in it (not empty). > > > > > > > > > > Could kindly explain more so I would learn more about this scenario? > > > > > Thanks a lot! > > > > > > > > i_nlink == 1 just means that there is one non-deleted link. There can > > > > be links > > > > that have since been deleted, and file descriptors can still be open to > > > > them. > > > > > > Thanks for your inspiration. You are right, thanks. > > > > > > Correct my words... I didn't check f2fs code just now, it seems f2fs > > > doesn't > > > take inode_lock as some other fs like __generic_file_fsync or ubifs_fsync. > > > > > > And i_sem locks nlink / try_to_fix_pino similarly in some extent. It seems > > > no race by using d_find_alias here. Thanks again. > > > > > > > (think more little bit just now...) > > > > Thread 1: Thread 2 (fsync): > > vfs_unlink try_to_fix_pino > > f2fs_unlink > >f2fs_delete_entry > > f2fs_drop_nlink (i_sem, inode->i_nlink = 1) > > > > (... but this dentry still hashed) i_sem, check > > inode->i_nlink = 1 > > i_sem d_find_alias > > > > d_delete > > > > I'm not sure if fsync could still use some wrong alias by chance.. > > completely untested, maybe just noise... > > > > Right, good observation. My patch makes it better, but it's still broken. > > I don't know how to fix it. If we see i_nlink == 1 and multiple hashed > dentries, there doesn't appear to be a way to distingush which one corresponds > to the remaining link on-disk (if any; it may not even be in the dcache), and > which correspond to links that vfs_unlink() has deleted from disk but hasn't > yet > done d_delete() on. > > One idea would be choose one, then take inode_lock_shared(dir) and do > __f2fs_find_entry() to check if the dentry is really still on-disk. That's > heavyweight and error-prone though, and the locking could cause problems. > > I'm wondering though, does f2fs really need try_to_fix_pino() at all, and did > it > ever really work? It never actually updates the f2fs_inode::i_name to match > the > new directory. So independently of this bug with deleted links, I don't see > how > it can possibly work as intended. Part of my humble opinion would be "update pino in rename/unlink/link... such ops instead of in fsync" (maybe it makes better sense of locking)... But actually I'm not a f2fs folk now, just curious about what the original patch resolved with these new extra igrab/iput (as I said before, I could not find some clue previously). Thanks, Gao Xiang > > - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: get parent inode when recovering pino
On Wed, May 06, 2020 at 02:47:19PM +0800, Gao Xiang wrote: > On Wed, May 06, 2020 at 09:58:22AM +0800, Gao Xiang wrote: > > On Tue, May 05, 2020 at 06:24:28PM -0700, Eric Biggers wrote: > > > On Wed, May 06, 2020 at 08:14:07AM +0800, Gao Xiang wrote: > > > > > > > > > > Actually, I think this is wrong because the fsync can be done via a > > > > > file > > > > > descriptor that was opened to a now-deleted link to the file. > > > > > > > > I'm still confused about this... > > > > > > > > I don't know what's wrong with this version from my limited knowledge? > > > > inode itself is locked when fsyncing, so > > > > > > > >if the fsync inode->i_nlink == 1, this inode has only one hard link > > > >(not deleted yet) and should belong to a single directory; and > > > > > > > >the only one parent directory would not go away (not deleted as well) > > > >since there are some dirents in it (not empty). > > > > > > > > Could kindly explain more so I would learn more about this scenario? > > > > Thanks a lot! > > > > > > i_nlink == 1 just means that there is one non-deleted link. There can be > > > links > > > that have since been deleted, and file descriptors can still be open to > > > them. > > > > Thanks for your inspiration. You are right, thanks. > > > > Correct my words... I didn't check f2fs code just now, it seems f2fs doesn't > > take inode_lock as some other fs like __generic_file_fsync or ubifs_fsync. > > > > And i_sem locks nlink / try_to_fix_pino similarly in some extent. It seems > > no race by using d_find_alias here. Thanks again. > > > > (think more little bit just now...) > > Thread 1: Thread 2 (fsync): > vfs_unlink try_to_fix_pino > f2fs_unlink >f2fs_delete_entry > f2fs_drop_nlink (i_sem, inode->i_nlink = 1) > > (... but this dentry still hashed) i_sem, check > inode->i_nlink = 1 > i_sem d_find_alias > > d_delete > > I'm not sure if fsync could still use some wrong alias by chance.. > completely untested, maybe just noise... > Right, good observation. My patch makes it better, but it's still broken. I don't know how to fix it. If we see i_nlink == 1 and multiple hashed dentries, there doesn't appear to be a way to distingush which one corresponds to the remaining link on-disk (if any; it may not even be in the dcache), and which correspond to links that vfs_unlink() has deleted from disk but hasn't yet done d_delete() on. One idea would be choose one, then take inode_lock_shared(dir) and do __f2fs_find_entry() to check if the dentry is really still on-disk. That's heavyweight and error-prone though, and the locking could cause problems. I'm wondering though, does f2fs really need try_to_fix_pino() at all, and did it ever really work? It never actually updates the f2fs_inode::i_name to match the new directory. So independently of this bug with deleted links, I don't see how it can possibly work as intended. - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: don't return vmalloc() memory from f2fs_kmalloc()
On Wed, May 06, 2020 at 03:43:36PM +0800, Chao Yu wrote: > On 2020/5/6 4:48, Eric Biggers wrote: > > From: Eric Biggers > > > > kmalloc() returns kmalloc'ed memory, and kvmalloc() returns either > > kmalloc'ed or vmalloc'ed memory. But the f2fs wrappers, f2fs_kmalloc() > > and f2fs_kvmalloc(), both return both kinds of memory. > > > > It's redundant to have two functions that do the same thing, and also > > breaking the standard naming convention is causing bugs since people > > assume it's safe to kfree() memory allocated by f2fs_kmalloc(). See > > e.g. the various allocations in fs/f2fs/compress.c. > > > > Fix this by making f2fs_kmalloc() just use kmalloc(). And to avoid > > re-introducing the allocation failures that the vmalloc fallback was > > intended to fix, convert the largest allocations to use f2fs_kvmalloc(). > > I've submitted one patch since you suggested when commented in compression > support patch. > > I remember Jaegeuk prefer to use f2fs_kvmalloc() to instead f2fs_kmalloc(), > and keep the order of kmalloc - failed - kvmalloc. > I think you're talking about https://lkml.kernel.org/r/20191105030451.ga55...@jaegeuk-macbookpro.roam.corp.google.com? I think that making the large allocations use kvmalloc(), as this patch does, is good enough to address any memory allocation failures that may have been encountered in the past. We don't need to switch all allocations to use kvmalloc(), as there's no benefit for small allocations. - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v2] f2fs: shrink spinlock coverage
On 05/06, Chao Yu wrote:
> In f2fs_try_to_free_nids(), .nid_list_lock spinlock critical region will
> increase as expected shrink number increase, to avoid spining other CPUs
> for long time, it's better to implement like extent cache and nats
> shrinker.
>
> Signed-off-by: Chao Yu
> ---
> v2:
> - fix unlock wrong spinlock.
> fs/f2fs/node.c | 15 +++
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 4da0d8713df5..ad0b14f4dab8 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -2488,7 +2488,6 @@ void f2fs_alloc_nid_failed(struct f2fs_sb_info *sbi,
> nid_t nid)
> int f2fs_try_to_free_nids(struct f2fs_sb_info *sbi, int nr_shrink)
> {
> struct f2fs_nm_info *nm_i = NM_I(sbi);
> - struct free_nid *i, *next;
> int nr = nr_shrink;
>
> if (nm_i->nid_cnt[FREE_NID] <= MAX_FREE_NIDS)
> @@ -2498,14 +2497,22 @@ int f2fs_try_to_free_nids(struct f2fs_sb_info *sbi,
> int nr_shrink)
> return 0;
>
> spin_lock(_i->nid_list_lock);
> - list_for_each_entry_safe(i, next, _i->free_nid_list, list) {
> - if (nr_shrink <= 0 ||
> - nm_i->nid_cnt[FREE_NID] <= MAX_FREE_NIDS)
> + while (nr_shrink) {
> + struct free_nid *i;
> +
> + if (nm_i->nid_cnt[FREE_NID] <= MAX_FREE_NIDS)
> break;
>
> + i = list_first_entry(_i->free_nid_list,
> + struct free_nid, list);
> + list_del(>list);
> + spin_unlock(_i->nid_list_lock);
> +
> __remove_free_nid(sbi, i, FREE_NID);
__remove_free_nid() will do list_del again. btw, how about just splitting out
given nr_shrink into multiple trials?
> kmem_cache_free(free_nid_slab, i);
> nr_shrink--;
> +
> + spin_lock(_i->nid_list_lock);
> }
> spin_unlock(_i->nid_list_lock);
> mutex_unlock(_i->build_lock);
> --
> 2.18.0.rc1
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: change maximum zstd compression buffer size
On 05/06, Chao Yu wrote: > On 2020/5/6 7:05, Jaegeuk Kim wrote: > > On 05/05, Chao Yu wrote: > >> On 2020-5-4 22:30, Jaegeuk Kim wrote: > >>> From: Daeho Jeong > >>> > >>> Current zstd compression buffer size is one page and header size less > >>> than cluster size. By this, zstd compression always succeeds even if > >>> the real compression data is failed to fit into the buffer size, and > >>> eventually reading the cluster returns I/O error with the corrupted > >>> compression data. > >> > >> What's the root cause of this issue? I didn't get it. > >> > >>> > >>> Signed-off-by: Daeho Jeong > >>> --- > >>> fs/f2fs/compress.c | 4 ++-- > >>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c > >>> index 4c7eaeee52336..a9fa8049b295f 100644 > >>> --- a/fs/f2fs/compress.c > >>> +++ b/fs/f2fs/compress.c > >>> @@ -313,7 +313,7 @@ static int zstd_init_compress_ctx(struct compress_ctx > >>> *cc) > >>> cc->private = workspace; > >>> cc->private2 = stream; > >>> > >>> - cc->clen = cc->rlen - PAGE_SIZE - COMPRESS_HEADER_SIZE; > >>> + cc->clen = ZSTD_compressBound(PAGE_SIZE << cc->log_cluster_size); > >> > >> In my machine, the value is 66572 which is much larger than size of dst > >> buffer, so, in where we can tell the real size of dst buffer to zstd > >> compressor? Otherwise, if compressed data size is larger than dst buffer > >> size, when we flush compressed data into dst buffer, we may suffer > >> overflow. > > > > Could you give it a try compress_log_size=2 and check decompression works? > > I tried some samples before submitting the patch, did you encounter app's data > corruption when using zstd algorithm? If so, let me check this issue. Daeho reported: 1. cp -a src_file comp_dir/dst_file (comp_dir is a directory for compression) 2. sync comp_dir/dst_file 3. echo 3 > /proc/sys/vm/drop_caches 4. cat comp_dir/dst_file > dump (it returns I/O error depending on the file). > > Thanks, > > > > >> > >>> return 0; > >>> } > >>> > >>> @@ -330,7 +330,7 @@ static int zstd_compress_pages(struct compress_ctx > >>> *cc) > >>> ZSTD_inBuffer inbuf; > >>> ZSTD_outBuffer outbuf; > >>> int src_size = cc->rlen; > >>> - int dst_size = src_size - PAGE_SIZE - COMPRESS_HEADER_SIZE; > >>> + int dst_size = cc->clen; > >>> int ret; > >>> > >>> inbuf.pos = 0; > >>> > > > > > > ___ > > Linux-f2fs-devel mailing list > > Linux-f2fs-devel@lists.sourceforge.net > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > > . > > ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: use strcmp() in parse_options()
On 05/06, Chao Yu wrote:
> On 2020/5/2 7:35, Eric Biggers wrote:
> > From: Eric Biggers
> >
> > Remove the pointless string length checks. Just use strcmp().
> >
> > Signed-off-by: Eric Biggers
>
> Reviewed-by: Chao Yu
>
> [...]
>
> > - } else if (strlen(name) == 4 &&
> > - !strcmp(name, "zstd")) {
> > + } else if (!strcmp(name, "zstd")) {
> > F2FS_OPTION(sbi).compress_algorithm =
> > COMPRESS_ZSTD;
>
> lzo-rle will be added, Jaegeuk, please notice that the late applied patch
> should adjust a bit here.
Sure.
>
> Thanks,
>
> > } else {
> >
>
>
> ___
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 3/3] f2fs_io: show more flags
On 05/06, Chao Yu wrote:
> On 2020/5/2 8:29, Jaegeuk Kim wrote:
> > Signed-off-by: Jaegeuk Kim
> > ---
> > tools/f2fs_io/f2fs_io.c | 28
> > tools/f2fs_io/f2fs_io.h | 12
> > 2 files changed, 40 insertions(+)
> >
> > diff --git a/tools/f2fs_io/f2fs_io.c b/tools/f2fs_io/f2fs_io.c
> > index 9be99f0..d1889ff 100644
> > --- a/tools/f2fs_io/f2fs_io.c
> > +++ b/tools/f2fs_io/f2fs_io.c
> > @@ -186,6 +186,10 @@ static void do_set_verity(int argc, char **argv, const
> > struct cmd_desc *cmd)
> > "f2fs_io getflags [file]\n\n" \
> > "get a flag given the file\n" \
> > "flag can show \n" \
> > +" encryption\n" \
> > +" nocow(pinned)\n"\
> > +" inline_data\n" \
> > +" verity\n" \
> > " casefold\n" \
> > " compression\n" \
> > " nocompression\n"
> > @@ -222,6 +226,30 @@ static void do_getflags(int argc, char **argv, const
> > struct cmd_desc *cmd)
> > printf("nocompression");
> > exist = 1;
> > }
> > + if (flag & FS_ENCRYPT_FL) {
> > + if (exist)
> > + printf(",");
> > + printf("encrypt");
> > + exist = 1;
> > + }
> > + if (flag & FS_VERITY_FL) {
> > + if (exist)
> > + printf(",");
> > + printf("verity");
> > + exist = 1;
> > + }
> > + if (flag & FS_INLINE_DATA_FL) {
> > + if (exist)
> > + printf(",");
> > + printf("inline_data");
> > + exist = 1;
> > + }
> > + if (flag & FS_NOCOW_FL) {
> > + if (exist)
> > + printf(",");
> > + printf("nocow(pinned)");
> > + exist = 1;
> > + }
> > if (!exist)
> > printf("none");
> > printf("\n");
> > diff --git a/tools/f2fs_io/f2fs_io.h b/tools/f2fs_io/f2fs_io.h
> > index c6ea7ff..2c828bc 100644
> > --- a/tools/f2fs_io/f2fs_io.h
> > +++ b/tools/f2fs_io/f2fs_io.h
> > @@ -110,6 +110,18 @@ typedef u32__be32;
> > #define F2FS_IOC_FSGETXATTRFS_IOC_FSGETXATTR
> > #define F2FS_IOC_FSSETXATTRFS_IOC_FSSETXATTR
> >
> > +#ifndef FS_ENCRYPT_FL
> > +#define FS_ENCRYPT_FL 0x0800 /* Encrypted file */
> > +#endif
> > +#ifndef FS_VERITY_FL
> > +#define FS_VERITY_FL 0x0010 /* Verity protected
> > inode */
> > +#endif
> > +#ifndef FS_INLINE_DATA_FL
> > +#define FS_INLINE_DATA_FL 0x1000 /* Reserved for ext4 */
>
> /* Inline data regular/symlink */
Done.
>
> > +#endif
> > +#ifndef FS_NOCOW_FL
> > +#define FS_NOCOW_FL0x0080 /* Do not cow file */
> > +#endif
> > #ifndef FS_NOCOMP_FL
> > #define FS_NOCOMP_FL 0x0400 /* Don't compress */
> > #endif
> >
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 1/3] f2fs_io: add fsync
On 05/06, Chao Yu wrote:
> On 2020/5/2 8:29, Jaegeuk Kim wrote:
> > Signed-off-by: Jaegeuk Kim
> > ---
> > tools/f2fs_io/f2fs_io.c | 25 +
> > 1 file changed, 25 insertions(+)
> >
> > diff --git a/tools/f2fs_io/f2fs_io.c b/tools/f2fs_io/f2fs_io.c
> > index c1edef1..c84b6ab 100644
> > --- a/tools/f2fs_io/f2fs_io.c
> > +++ b/tools/f2fs_io/f2fs_io.c
> > @@ -130,6 +130,30 @@ static void full_write(int fd, const void *buf, size_t
> > count)
> > }
> > }
> >
> > +#define fsync_desc "fsync"
> > +#define fsync_help \
> > +"f2fs_io fsync [file]\n\n" \
>
> What about supporting fdatasync via an additional argument here?
I prefer to add another command "fdatasync" for simplicity. :P
>
> > +"fsync given the file\n" \
> > +
> > +static void do_fsync(int argc, char **argv, const struct cmd_desc *cmd)
> > +{
> > + int fd;
> > +
> > + if (argc != 2) {
> > + fputs("Excess arguments\n\n", stderr);
> > + fputs(cmd->cmd_help, stderr);
> > + exit(1);
> > + }
> > +
> > + fd = xopen(argv[1], O_WRONLY, 0);
> > +
> > + if (fsync(fd) != 0)
> > + die_errno("fsync failed");
> > +
> > + printf("fsync a file\n");
> > + exit(0);
> > +}
> > +
> > #define set_verity_desc "Set fs-verity"
> > #define set_verity_help\
> > "f2fs_io set_verity [file]\n\n"\
> > @@ -780,6 +804,7 @@ static void do_reserve_cblocks(int argc, char **argv,
> > const struct cmd_desc *cmd
> > static void do_help(int argc, char **argv, const struct cmd_desc *cmd);
> > const struct cmd_desc cmd_list[] = {
> > _CMD(help),
> > + CMD(fsync),
> > CMD(set_verity),
> > CMD(getflags),
> > CMD(setflags),
> >
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH v2] f2fs: shrink spinlock coverage
In f2fs_try_to_free_nids(), .nid_list_lock spinlock critical region will
increase as expected shrink number increase, to avoid spining other CPUs
for long time, it's better to implement like extent cache and nats
shrinker.
Signed-off-by: Chao Yu
---
v2:
- fix unlock wrong spinlock.
fs/f2fs/node.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 4da0d8713df5..ad0b14f4dab8 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -2488,7 +2488,6 @@ void f2fs_alloc_nid_failed(struct f2fs_sb_info *sbi,
nid_t nid)
int f2fs_try_to_free_nids(struct f2fs_sb_info *sbi, int nr_shrink)
{
struct f2fs_nm_info *nm_i = NM_I(sbi);
- struct free_nid *i, *next;
int nr = nr_shrink;
if (nm_i->nid_cnt[FREE_NID] <= MAX_FREE_NIDS)
@@ -2498,14 +2497,22 @@ int f2fs_try_to_free_nids(struct f2fs_sb_info *sbi, int
nr_shrink)
return 0;
spin_lock(_i->nid_list_lock);
- list_for_each_entry_safe(i, next, _i->free_nid_list, list) {
- if (nr_shrink <= 0 ||
- nm_i->nid_cnt[FREE_NID] <= MAX_FREE_NIDS)
+ while (nr_shrink) {
+ struct free_nid *i;
+
+ if (nm_i->nid_cnt[FREE_NID] <= MAX_FREE_NIDS)
break;
+ i = list_first_entry(_i->free_nid_list,
+ struct free_nid, list);
+ list_del(>list);
+ spin_unlock(_i->nid_list_lock);
+
__remove_free_nid(sbi, i, FREE_NID);
kmem_cache_free(free_nid_slab, i);
nr_shrink--;
+
+ spin_lock(_i->nid_list_lock);
}
spin_unlock(_i->nid_list_lock);
mutex_unlock(_i->build_lock);
--
2.18.0.rc1
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH] f2fs: shrink spinlock coverage
In f2fs_try_to_free_nids(), .nid_list_lock spinlock critical region will
increase as expected shrink number increase, to avoid spining other CPUs
for long time, it's better to implement like extent cache and nats
shrinker.
Signed-off-by: Chao Yu
---
fs/f2fs/node.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 4da0d8713df5..a0cf66ace41e 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -2488,7 +2488,6 @@ void f2fs_alloc_nid_failed(struct f2fs_sb_info *sbi,
nid_t nid)
int f2fs_try_to_free_nids(struct f2fs_sb_info *sbi, int nr_shrink)
{
struct f2fs_nm_info *nm_i = NM_I(sbi);
- struct free_nid *i, *next;
int nr = nr_shrink;
if (nm_i->nid_cnt[FREE_NID] <= MAX_FREE_NIDS)
@@ -2498,14 +2497,22 @@ int f2fs_try_to_free_nids(struct f2fs_sb_info *sbi, int
nr_shrink)
return 0;
spin_lock(_i->nid_list_lock);
- list_for_each_entry_safe(i, next, _i->free_nid_list, list) {
- if (nr_shrink <= 0 ||
- nm_i->nid_cnt[FREE_NID] <= MAX_FREE_NIDS)
+ while (nr_shrink) {
+ struct free_nid *i;
+
+ if (nm_i->nid_cnt[FREE_NID] <= MAX_FREE_NIDS)
break;
+ i = list_first_entry(_i->free_nid_list,
+ struct free_nid, list);
+ list_del(>list);
+ spin_unlock(_i->nat_list_lock);
+
__remove_free_nid(sbi, i, FREE_NID);
kmem_cache_free(free_nid_slab, i);
nr_shrink--;
+
+ spin_lock(_i->nid_list_lock);
}
spin_unlock(_i->nid_list_lock);
mutex_unlock(_i->build_lock);
--
2.18.0.rc1
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: change maximum zstd compression buffer size
On 2020/5/6 7:05, Jaegeuk Kim wrote: > On 05/05, Chao Yu wrote: >> On 2020-5-4 22:30, Jaegeuk Kim wrote: >>> From: Daeho Jeong >>> >>> Current zstd compression buffer size is one page and header size less >>> than cluster size. By this, zstd compression always succeeds even if >>> the real compression data is failed to fit into the buffer size, and >>> eventually reading the cluster returns I/O error with the corrupted >>> compression data. >> >> What's the root cause of this issue? I didn't get it. >> >>> >>> Signed-off-by: Daeho Jeong >>> --- >>> fs/f2fs/compress.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c >>> index 4c7eaeee52336..a9fa8049b295f 100644 >>> --- a/fs/f2fs/compress.c >>> +++ b/fs/f2fs/compress.c >>> @@ -313,7 +313,7 @@ static int zstd_init_compress_ctx(struct compress_ctx >>> *cc) >>> cc->private = workspace; >>> cc->private2 = stream; >>> >>> - cc->clen = cc->rlen - PAGE_SIZE - COMPRESS_HEADER_SIZE; >>> + cc->clen = ZSTD_compressBound(PAGE_SIZE << cc->log_cluster_size); >> >> In my machine, the value is 66572 which is much larger than size of dst >> buffer, so, in where we can tell the real size of dst buffer to zstd >> compressor? Otherwise, if compressed data size is larger than dst buffer >> size, when we flush compressed data into dst buffer, we may suffer overflow. > > Could you give it a try compress_log_size=2 and check decompression works? I tried some samples before submitting the patch, did you encounter app's data corruption when using zstd algorithm? If so, let me check this issue. Thanks, > >> >>> return 0; >>> } >>> >>> @@ -330,7 +330,7 @@ static int zstd_compress_pages(struct compress_ctx *cc) >>> ZSTD_inBuffer inbuf; >>> ZSTD_outBuffer outbuf; >>> int src_size = cc->rlen; >>> - int dst_size = src_size - PAGE_SIZE - COMPRESS_HEADER_SIZE; >>> + int dst_size = cc->clen; >>> int ret; >>> >>> inbuf.pos = 0; >>> > > > ___ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > . > ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: don't return vmalloc() memory from f2fs_kmalloc()
On 2020/5/6 4:48, Eric Biggers wrote:
> From: Eric Biggers
>
> kmalloc() returns kmalloc'ed memory, and kvmalloc() returns either
> kmalloc'ed or vmalloc'ed memory. But the f2fs wrappers, f2fs_kmalloc()
> and f2fs_kvmalloc(), both return both kinds of memory.
>
> It's redundant to have two functions that do the same thing, and also
> breaking the standard naming convention is causing bugs since people
> assume it's safe to kfree() memory allocated by f2fs_kmalloc(). See
> e.g. the various allocations in fs/f2fs/compress.c.
>
> Fix this by making f2fs_kmalloc() just use kmalloc(). And to avoid
> re-introducing the allocation failures that the vmalloc fallback was
> intended to fix, convert the largest allocations to use f2fs_kvmalloc().
I've submitted one patch since you suggested when commented in compression
support patch.
I remember Jaegeuk prefer to use f2fs_kvmalloc() to instead f2fs_kmalloc(),
and keep the order of kmalloc - failed - kvmalloc.
Thanks,
>
> Signed-off-by: Eric Biggers
> ---
> fs/f2fs/checkpoint.c | 4 ++--
> fs/f2fs/f2fs.h | 8 +---
> fs/f2fs/node.c | 8
> 3 files changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 97b6378554b406..ac5b47f15f5e77 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -895,8 +895,8 @@ int f2fs_get_valid_checkpoint(struct f2fs_sb_info *sbi)
> int i;
> int err;
>
> - sbi->ckpt = f2fs_kzalloc(sbi, array_size(blk_size, cp_blks),
> - GFP_KERNEL);
> + sbi->ckpt = f2fs_kvzalloc(sbi, array_size(blk_size, cp_blks),
> + GFP_KERNEL);
> if (!sbi->ckpt)
> return -ENOMEM;
> /*
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index d036a31a97e84e..bc4c5b9b1bf14c 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -2953,18 +2953,12 @@ static inline bool f2fs_may_extent_tree(struct inode
> *inode)
> static inline void *f2fs_kmalloc(struct f2fs_sb_info *sbi,
> size_t size, gfp_t flags)
> {
> - void *ret;
> -
> if (time_to_inject(sbi, FAULT_KMALLOC)) {
> f2fs_show_injection_info(sbi, FAULT_KMALLOC);
> return NULL;
> }
>
> - ret = kmalloc(size, flags);
> - if (ret)
> - return ret;
> -
> - return kvmalloc(size, flags);
> + return kmalloc(size, flags);
> }
>
> static inline void *f2fs_kzalloc(struct f2fs_sb_info *sbi,
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 4da0d8713df5cb..e660af55565c61 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -2934,7 +2934,7 @@ static int __get_nat_bitmaps(struct f2fs_sb_info *sbi)
> return 0;
>
> nm_i->nat_bits_blocks = F2FS_BLK_ALIGN((nat_bits_bytes << 1) + 8);
> - nm_i->nat_bits = f2fs_kzalloc(sbi,
> + nm_i->nat_bits = f2fs_kvzalloc(sbi,
> nm_i->nat_bits_blocks << F2FS_BLKSIZE_BITS, GFP_KERNEL);
> if (!nm_i->nat_bits)
> return -ENOMEM;
> @@ -3067,9 +3067,9 @@ static int init_free_nid_cache(struct f2fs_sb_info *sbi)
> int i;
>
> nm_i->free_nid_bitmap =
> - f2fs_kzalloc(sbi, array_size(sizeof(unsigned char *),
> - nm_i->nat_blocks),
> - GFP_KERNEL);
> + f2fs_kvzalloc(sbi, array_size(sizeof(unsigned char *),
> + nm_i->nat_blocks),
> + GFP_KERNEL);
> if (!nm_i->free_nid_bitmap)
> return -ENOMEM;
>
>
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: get parent inode when recovering pino
On 2020/5/6 9:24, Eric Biggers wrote:
> On Wed, May 06, 2020 at 08:14:07AM +0800, Gao Xiang wrote:
>>>
>>> Actually, I think this is wrong because the fsync can be done via a file
>>> descriptor that was opened to a now-deleted link to the file.
>>
>> I'm still confused about this...
>>
>> I don't know what's wrong with this version from my limited knowledge?
>> inode itself is locked when fsyncing, so
>>
>>if the fsync inode->i_nlink == 1, this inode has only one hard link
>>(not deleted yet) and should belong to a single directory; and
>>
>>the only one parent directory would not go away (not deleted as well)
>>since there are some dirents in it (not empty).
>>
>> Could kindly explain more so I would learn more about this scenario?
>> Thanks a lot!
>
> i_nlink == 1 just means that there is one non-deleted link. There can be
> links
> that have since been deleted, and file descriptors can still be open to them.
>
>>
>>>
>>> We need to find the dentry whose parent directory is still exists, i.e. the
>>> parent directory that is counting towards 'inode->i_nlink == 1'.
>>
>> directory counting towards 'inode->i_nlink == 1', what's happening?
>
> The non-deleted link is the one counted in i_nlink.
>
>>
>>>
>>> I think d_find_alias() is what we're looking for.
>>
>> It may be simply dentry->d_parent (stable/positive as you said before, and
>> it's
>> not empty). why need to d_find_alias()?
>
> Because we need to get the dentry that hasn't been deleted yet, which isn't
> necessarily the one associated with the file descriptor being fsync()'ed.
>
>> And what is the original problem? I could not get some clue from the original
>> patch description (I only saw some extra igrab/iput because of some unknown
>> reasons), it there some backtrace related to the problem?
>
> The problem is that i_pino gets set incorrectly. I just noticed this while
> reviewing the code. It's not hard to reproduce, e.g.:
>
> #include
> #include
> #include
>
> int main()
> {
> int fd;
>
> mkdir("dir1", 0700);
> mkdir("dir2", 0700);
> mknod("dir1/file", S_IFREG|0600, 0);
> link("dir1/file", "dir2/file");
> fd = open("dir2/file", O_WRONLY);
> unlink("dir2/file");
> write(fd, "X", 1);
> fsync(fd);
> }
>
> Then:
>
> sync
> echo N | dump.f2fs -i $(stat -c %i dir1/file) /dev/vdb | grep 'i_pino'
> echo "dir1 (correct): $(stat -c %i dir1)"
> echo "dir2 (wrong): $(stat -c %i dir2)"
>
> i_pino will point to dir2 rather than dir1 as expected.
Could you add above testcase into commit message of your patch? it will
be easier to understand the issue we solved with it.
In addition, how about adding this testcase in fstest as a generic one?
>
> - Eric
>
>
> ___
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> .
>
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: remove redundant check in f2fs_force_buffered_io
On 2020/5/5 11:23, Jaegeuk Kim wrote: > On 05/05, Chao Yu wrote: >> On 2020-5-4 22:35, Jaegeuk Kim wrote: >>> From: Daeho Jeong >>> >>> We already checked whether the file is compressed or not in >>> f2fs_post_read_required(). So removed f2fs_compressed_file() >>> in f2fs_force_buffered_io(). >> >> Agreed, since I have sent similar patch before: >> >> https://lkml.org/lkml/2020/3/24/1819 > > Heh, as I couldn't find yours, I was actually waiting for you to point out. :) Well, all patches sent to f2fs mailing list have been archived in lore.kernel.org/linux-f2fs-devel. :) https://lore.kernel.org/linux-f2fs-devel/20200229104906.12061-1-yuch...@huawei.com/ > >> >> Just want to know what's the change of backport concern now. > > Old ICE support had to decouple f2fs_post_read_required() and > f2fs_forced_buffered_io(). Now, I decide to manage this as we Copied. > need to manage this for one kernel version only. Okay, thanks for the explanation. Thanks, > > Thanks, > >> >> Thanks, >> >>> >>> Signed-off-by: Daeho Jeong >>> Signed-off-by: Jaegeuk Kim >>> --- >>> fs/f2fs/f2fs.h | 2 -- >>> 1 file changed, 2 deletions(-) >>> >>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >>> index 6b7b963641696..01a00fc808361 100644 >>> --- a/fs/f2fs/f2fs.h >>> +++ b/fs/f2fs/f2fs.h >>> @@ -4064,8 +4064,6 @@ static inline bool f2fs_force_buffered_io(struct >>> inode *inode, >>> return true; >>> if (f2fs_is_multi_device(sbi)) >>> return true; >>> - if (f2fs_compressed_file(inode)) >>> - return true; >>> /* >>> * for blkzoned device, fallback direct IO to buffered IO, so >>> * all IOs can be serialized by log-structured write. >>> > > > ___ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > . > ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: get parent inode when recovering pino
On Wed, May 06, 2020 at 09:58:22AM +0800, Gao Xiang wrote: > On Tue, May 05, 2020 at 06:24:28PM -0700, Eric Biggers wrote: > > On Wed, May 06, 2020 at 08:14:07AM +0800, Gao Xiang wrote: > > > > > > > > Actually, I think this is wrong because the fsync can be done via a file > > > > descriptor that was opened to a now-deleted link to the file. > > > > > > I'm still confused about this... > > > > > > I don't know what's wrong with this version from my limited knowledge? > > > inode itself is locked when fsyncing, so > > > > > >if the fsync inode->i_nlink == 1, this inode has only one hard link > > >(not deleted yet) and should belong to a single directory; and > > > > > >the only one parent directory would not go away (not deleted as well) > > >since there are some dirents in it (not empty). > > > > > > Could kindly explain more so I would learn more about this scenario? > > > Thanks a lot! > > > > i_nlink == 1 just means that there is one non-deleted link. There can be > > links > > that have since been deleted, and file descriptors can still be open to > > them. > > Thanks for your inspiration. You are right, thanks. > > Correct my words... I didn't check f2fs code just now, it seems f2fs doesn't > take inode_lock as some other fs like __generic_file_fsync or ubifs_fsync. > > And i_sem locks nlink / try_to_fix_pino similarly in some extent. It seems > no race by using d_find_alias here. Thanks again. > (think more little bit just now...) Thread 1: Thread 2 (fsync): vfs_unlink try_to_fix_pino f2fs_unlink f2fs_delete_entry f2fs_drop_nlink (i_sem, inode->i_nlink = 1) (... but this dentry still hashed) i_sem, check inode->i_nlink = 1 i_sem d_find_alias d_delete I'm not sure if fsync could still use some wrong alias by chance.. completely untested, maybe just noise... Thanks, Gao Xiang ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 3/3] f2fs_io: show more flags
On 2020/5/2 8:29, Jaegeuk Kim wrote:
> Signed-off-by: Jaegeuk Kim
> ---
> tools/f2fs_io/f2fs_io.c | 28
> tools/f2fs_io/f2fs_io.h | 12
> 2 files changed, 40 insertions(+)
>
> diff --git a/tools/f2fs_io/f2fs_io.c b/tools/f2fs_io/f2fs_io.c
> index 9be99f0..d1889ff 100644
> --- a/tools/f2fs_io/f2fs_io.c
> +++ b/tools/f2fs_io/f2fs_io.c
> @@ -186,6 +186,10 @@ static void do_set_verity(int argc, char **argv, const
> struct cmd_desc *cmd)
> "f2fs_io getflags [file]\n\n"\
> "get a flag given the file\n"\
> "flag can show \n" \
> +" encryption\n" \
> +" nocow(pinned)\n" \
> +" inline_data\n"\
> +" verity\n" \
> " casefold\n" \
> " compression\n"\
> " nocompression\n"
> @@ -222,6 +226,30 @@ static void do_getflags(int argc, char **argv, const
> struct cmd_desc *cmd)
> printf("nocompression");
> exist = 1;
> }
> + if (flag & FS_ENCRYPT_FL) {
> + if (exist)
> + printf(",");
> + printf("encrypt");
> + exist = 1;
> + }
> + if (flag & FS_VERITY_FL) {
> + if (exist)
> + printf(",");
> + printf("verity");
> + exist = 1;
> + }
> + if (flag & FS_INLINE_DATA_FL) {
> + if (exist)
> + printf(",");
> + printf("inline_data");
> + exist = 1;
> + }
> + if (flag & FS_NOCOW_FL) {
> + if (exist)
> + printf(",");
> + printf("nocow(pinned)");
> + exist = 1;
> + }
> if (!exist)
> printf("none");
> printf("\n");
> diff --git a/tools/f2fs_io/f2fs_io.h b/tools/f2fs_io/f2fs_io.h
> index c6ea7ff..2c828bc 100644
> --- a/tools/f2fs_io/f2fs_io.h
> +++ b/tools/f2fs_io/f2fs_io.h
> @@ -110,6 +110,18 @@ typedef u32 __be32;
> #define F2FS_IOC_FSGETXATTR FS_IOC_FSGETXATTR
> #define F2FS_IOC_FSSETXATTR FS_IOC_FSSETXATTR
>
> +#ifndef FS_ENCRYPT_FL
> +#define FS_ENCRYPT_FL0x0800 /* Encrypted file */
> +#endif
> +#ifndef FS_VERITY_FL
> +#define FS_VERITY_FL 0x0010 /* Verity protected inode */
> +#endif
> +#ifndef FS_INLINE_DATA_FL
> +#define FS_INLINE_DATA_FL0x1000 /* Reserved for ext4 */
/* Inline data regular/symlink */
> +#endif
> +#ifndef FS_NOCOW_FL
> +#define FS_NOCOW_FL 0x0080 /* Do not cow file */
> +#endif
> #ifndef FS_NOCOMP_FL
> #define FS_NOCOMP_FL 0x0400 /* Don't compress */
> #endif
>
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 2/3] f2fs_io: don't give garbage data in upper 32bits
On 2020/5/2 8:29, Jaegeuk Kim wrote: > Signed-off-by: Jaegeuk Kim Reviewed-by: Chao Yu Thanks, ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 1/3] f2fs_io: add fsync
On 2020/5/2 8:29, Jaegeuk Kim wrote:
> Signed-off-by: Jaegeuk Kim
> ---
> tools/f2fs_io/f2fs_io.c | 25 +
> 1 file changed, 25 insertions(+)
>
> diff --git a/tools/f2fs_io/f2fs_io.c b/tools/f2fs_io/f2fs_io.c
> index c1edef1..c84b6ab 100644
> --- a/tools/f2fs_io/f2fs_io.c
> +++ b/tools/f2fs_io/f2fs_io.c
> @@ -130,6 +130,30 @@ static void full_write(int fd, const void *buf, size_t
> count)
> }
> }
>
> +#define fsync_desc "fsync"
> +#define fsync_help \
> +"f2fs_io fsync [file]\n\n" \
What about supporting fdatasync via an additional argument here?
> +"fsync given the file\n" \
> +
> +static void do_fsync(int argc, char **argv, const struct cmd_desc *cmd)
> +{
> + int fd;
> +
> + if (argc != 2) {
> + fputs("Excess arguments\n\n", stderr);
> + fputs(cmd->cmd_help, stderr);
> + exit(1);
> + }
> +
> + fd = xopen(argv[1], O_WRONLY, 0);
> +
> + if (fsync(fd) != 0)
> + die_errno("fsync failed");
> +
> + printf("fsync a file\n");
> + exit(0);
> +}
> +
> #define set_verity_desc "Set fs-verity"
> #define set_verity_help \
> "f2fs_io set_verity [file]\n\n" \
> @@ -780,6 +804,7 @@ static void do_reserve_cblocks(int argc, char **argv,
> const struct cmd_desc *cmd
> static void do_help(int argc, char **argv, const struct cmd_desc *cmd);
> const struct cmd_desc cmd_list[] = {
> _CMD(help),
> + CMD(fsync),
> CMD(set_verity),
> CMD(getflags),
> CMD(setflags),
>
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: use strcmp() in parse_options()
On 2020/5/2 7:35, Eric Biggers wrote:
> From: Eric Biggers
>
> Remove the pointless string length checks. Just use strcmp().
>
> Signed-off-by: Eric Biggers
Reviewed-by: Chao Yu
[...]
> - } else if (strlen(name) == 4 &&
> - !strcmp(name, "zstd")) {
> + } else if (!strcmp(name, "zstd")) {
> F2FS_OPTION(sbi).compress_algorithm =
> COMPRESS_ZSTD;
lzo-rle will be added, Jaegeuk, please notice that the late applied patch
should adjust a bit here.
Thanks,
> } else {
>
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH V4] f2fs: Avoid double lock for cp_rwsem during checkpoint
On 2020/4/30 18:58, Sayali Lokhande wrote:
> There could be a scenario where f2fs_sync_node_pages gets
> called during checkpoint, which in turn tries to flush
> inline data and calls iput(). This results in deadlock as
> iput() tries to hold cp_rwsem, which is already held at the
> beginning by checkpoint->block_operations().
>
> Call stack :
>
> Thread A Thread B
> f2fs_write_checkpoint()
> - block_operations(sbi)
> - f2fs_lock_all(sbi);
> - down_write(>cp_rwsem);
>
> - open()
> - igrab()
> - write() write inline data
> - unlink()
> - f2fs_sync_node_pages()
> - if (is_inline_node(page))
> - flush_inline_data()
>- ilookup()
> page = f2fs_pagecache_get_page()
> if (!page)
> goto iput_out;
> iput_out:
> -close()
> -iput()
>iput(inode);
>- f2fs_evict_inode()
> - f2fs_truncate_blocks()
> - f2fs_lock_op()
>- down_read(>cp_rwsem);
>
> Fixes: 2049d4fcb057 ("f2fs: avoid multiple node page writes due to
> inline_data")
> Signed-off-by: Sayali Lokhande
Reviewed-by: Chao Yu
Thanks,
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
