Re: [f2fs-dev] [PATCH] f2fs_io: support unset subcommand for pinfile

2024-04-09 Thread Chao Yu 

Ping,

Missed to check this patch?

On 2024/3/29 18:25, Chao Yu wrote:

This patch adds unset subcommand for pinfile command.

Usage: f2fs_io pinfile unset [target_file]

Signed-off-by: Chao Yu 
---
  man/f2fs_io.8   |  2 +-
  tools/f2fs_io/f2fs_io.c | 11 +--
  2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/man/f2fs_io.8 b/man/f2fs_io.8
index f097bde..b9c9dc8 100644
--- a/man/f2fs_io.8
+++ b/man/f2fs_io.8
@@ -44,7 +44,7 @@ going down with metadata flush
  going down with fsck mark
  .RE
  .TP
-\fBpinfile\fR \fI[get|set] [file]\fR
+\fBpinfile\fR \fI[get|set|unset] [file]\fR
  Get or set the pinning status on a file.
  .TP
  \fBfadvise\fR \fI[advice] [offset] [length] [file]\fR
diff --git a/tools/f2fs_io/f2fs_io.c b/tools/f2fs_io/f2fs_io.c
index b8e4f02..a7b593a 100644
--- a/tools/f2fs_io/f2fs_io.c
+++ b/tools/f2fs_io/f2fs_io.c
@@ -442,7 +442,7 @@ static void do_fadvise(int argc, char **argv, const struct 
cmd_desc *cmd)
  
  #define pinfile_desc "pin file control"

  #define pinfile_help  \
-"f2fs_io pinfile [get|set] [file]\n\n"   \
+"f2fs_io pinfile [get|set|unset] [file]\n\n" \
  "get/set pinning given the file\n"  \
  
  static void do_pinfile(int argc, char **argv, const struct cmd_desc *cmd)

@@ -464,7 +464,14 @@ static void do_pinfile(int argc, char **argv, const struct 
cmd_desc *cmd)
ret = ioctl(fd, F2FS_IOC_SET_PIN_FILE, );
if (ret != 0)
die_errno("F2FS_IOC_SET_PIN_FILE failed");
-   printf("set_pin_file: %u blocks moved in %s\n", ret, argv[2]);
+   printf("%s pinfile: %u blocks moved in %s\n",
+   argv[1], ret, argv[2]);
+   } else if (!strcmp(argv[1], "unset")) {
+   pin = 0;
+   ret = ioctl(fd, F2FS_IOC_SET_PIN_FILE, );
+   if (ret != 0)
+   die_errno("F2FS_IOC_SET_PIN_FILE failed");
+   printf("%s pinfile in %s\n", argv[1], argv[2]);
} else if (!strcmp(argv[1], "get")) {
unsigned int flags;
  



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

Re: [f2fs-dev] [PATCH] resize.f2fs: get value from new sb during rebuilding cp

2024-04-09 Thread Sheng Yong via Linux-f2fs-devel 




On 2024/4/10 8:38, Jaegeuk Kim wrote:

On 04/09, Sheng Yong wrote:



On 2024/4/9 2:34, Jaegeuk Kim wrote:

On 04/08, Sheng Yong wrote:

Althrough old and new sb have the same value for now, it would be better
to build new checkpoint according to new sb.


May need to add assert, if they're different?


We could add assert here, but I think it's not that necessary:
1. rebuild_checkpoint is only called by resize, and new_sb is copied directly
from original sb without any changes of these basic attributes.
2. for now, new_sb has the same attributes/members with the original one. If
those attributes are allowed to get changed in the future, the assert needs
to be removed.
So how about adding a new helper to check and show the difference between the
new and original sb?


So, why do we need to change this?

Semantically, IMO, these fields belonging to new cp should be calculated based
on new sb, although the results are equal. That is found when I am trying to
make resize.f2fs support resizing with different attributes.

thanks,
shengyong




many thanks,
shengyong


Signed-off-by: Sheng Yong 
---
   fsck/resize.c | 7 ---
   1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fsck/resize.c b/fsck/resize.c
index 049ddd3..1b4ae85 100644
--- a/fsck/resize.c
+++ b/fsck/resize.c
@@ -481,7 +481,7 @@ static void rebuild_checkpoint(struct f2fs_sb_info *sbi,
set_cp(overprov_segment_count, get_cp(rsvd_segment_count));
set_cp(overprov_segment_count, get_cp(overprov_segment_count) +
-   2 * get_sb(segs_per_sec));
+   2 * get_newsb(segs_per_sec));
DBG(0, "Info: Overprovision ratio = %.3lf%%\n", c.new_overprovision);
DBG(0, "Info: Overprovision segments = %u (GC reserved = %u)\n",
@@ -551,11 +551,12 @@ static void rebuild_checkpoint(struct f2fs_sb_info *sbi,
cpu_to_le32(crc);
/* Write a new checkpoint in the other set */
-   new_cp_blk_no = old_cp_blk_no = get_sb(cp_blkaddr);
+   old_cp_blk_no = get_sb(cp_blkaddr);
+   new_cp_blk_no = get_newsb(cp_blkaddr);
if (sbi->cur_cp == 2)
old_cp_blk_no += 1 << get_sb(log_blocks_per_seg);
else
-   new_cp_blk_no += 1 << get_sb(log_blocks_per_seg);
+   new_cp_blk_no += 1 << get_newsb(log_blocks_per_seg);
/* write first cp */
ret = dev_write_block(new_cp, new_cp_blk_no++);
--
2.40.1



___
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: write missing last sum blk of file pinning section

2024-04-09 Thread Chao Yu 

On 2024/4/10 7:34, Daeho Jeong wrote:

From: Daeho Jeong 

While do not allocating a new section in advance for file pinning area, I
missed that we should write the sum block for the last segment of a file
pinning section.

Fixes: 9703d69d9d15 ("f2fs: support file pinning for zoned devices")
Signed-off-by: Daeho Jeong 


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] f2fs: don't set RO when shutting down f2fs

2024-04-09 Thread Chao Yu 

On 2024/4/10 0:21, Jaegeuk Kim wrote:

On 04/09, Chao Yu wrote:

On 2024/4/5 3:52, Jaegeuk Kim wrote:

Shutdown does not check the error of thaw_super due to readonly, which
causes a deadlock like below.

f2fs_ioc_shutdown(F2FS_GOING_DOWN_FULLSYNC)issue_discard_thread
   - bdev_freeze
- freeze_super
   - f2fs_stop_checkpoint()
- f2fs_handle_critical_error - sb_start_write
  - set RO - waiting
   - bdev_thaw
- thaw_super_locked
  - return -EINVAL, if sb_rdonly()
   - f2fs_stop_discard_thread
-> wait for kthread_stop(discard_thread);

Reported-by: "Light Hsieh (謝明燈)" 
Signed-off-by: Jaegeuk Kim 
---
   fs/f2fs/super.c | 11 +--
   1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index df9765b41dac..ba6288e870c5 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -4135,9 +4135,16 @@ void f2fs_handle_critical_error(struct f2fs_sb_info 
*sbi, unsigned char reason,
if (shutdown)
set_sbi_flag(sbi, SBI_IS_SHUTDOWN);
-   /* continue filesystem operators if errors=continue */
-   if (continue_fs || f2fs_readonly(sb))
+   /*
+* Continue filesystem operators if errors=continue. Should not set
+* RO by shutdown, since RO bypasses thaw_super which can hang the
+* system.
+*/
+   if (continue_fs || f2fs_readonly(sb) ||
+   reason == STOP_CP_REASON_SHUTDOWN) {
+   f2fs_warn(sbi, "Stopped filesystem due to readon: %d", reason);
return;


Do we need to set RO after bdev_thaw() in f2fs_do_shutdown()?


IIRC, shutdown doesn't need to set RO as we stopped the checkpoint.
I'm more concerned on any side effect caused by this RO change.


Okay, I just wonder whether we need to follow semantics of errors=remount-ro
semantics, but it looks fine since shutdown operation simulated by ioctl
could not be treated as an inner critical error,

errors=%sSpecify f2fs behavior on critical errors. This 
supports modes:
 "panic", "continue" and "remount-ro", respectively, 
trigger
 panic immediately, continue without doing anything, 
and remount
 the partition in read-only mode. By default it uses 
"continue"
 mode.

Also, it keeps the behavior consistent w/ what we do for errors=panic case.

if (F2FS_OPTION(sbi).errors == MOUNT_ERRORS_PANIC &&
!shutdown && !system_going_down() &&
^
!is_sbi_flag_set(sbi, SBI_IS_SHUTDOWN))
panic("F2FS-fs (device %s): panic forced after error\n",
sb->s_id);

Thanks,





Thanks,


+   }
f2fs_warn(sbi, "Remounting filesystem read-only");
/*



___
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: don't set RO when shutting down f2fs

2024-04-09 Thread Chao Yu 

On 2024/4/10 0:20, Jaegeuk Kim wrote:

Shutdown does not check the error of thaw_super due to readonly, which
causes a deadlock like below.

f2fs_ioc_shutdown(F2FS_GOING_DOWN_FULLSYNC)issue_discard_thread
  - bdev_freeze
   - freeze_super
  - f2fs_stop_checkpoint()
   - f2fs_handle_critical_error - sb_start_write
 - set RO - waiting
  - bdev_thaw
   - thaw_super_locked
 - return -EINVAL, if sb_rdonly()
  - f2fs_stop_discard_thread
   -> wait for kthread_stop(discard_thread);

Reported-by: "Light Hsieh (謝明燈)" 
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] resize.f2fs: get value from new sb during rebuilding cp

2024-04-09 Thread Jaegeuk Kim 
On 04/09, Sheng Yong wrote:
> 
> 
> On 2024/4/9 2:34, Jaegeuk Kim wrote:
> > On 04/08, Sheng Yong wrote:
> > > Althrough old and new sb have the same value for now, it would be better
> > > to build new checkpoint according to new sb.
> > 
> > May need to add assert, if they're different?
> > 
> We could add assert here, but I think it's not that necessary:
> 1. rebuild_checkpoint is only called by resize, and new_sb is copied directly
>from original sb without any changes of these basic attributes.
> 2. for now, new_sb has the same attributes/members with the original one. If
>those attributes are allowed to get changed in the future, the assert needs
>to be removed.
> So how about adding a new helper to check and show the difference between the
> new and original sb?

So, why do we need to change this?

> 
> many thanks,
> shengyong
> > > 
> > > Signed-off-by: Sheng Yong 
> > > ---
> > >   fsck/resize.c | 7 ---
> > >   1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/fsck/resize.c b/fsck/resize.c
> > > index 049ddd3..1b4ae85 100644
> > > --- a/fsck/resize.c
> > > +++ b/fsck/resize.c
> > > @@ -481,7 +481,7 @@ static void rebuild_checkpoint(struct f2fs_sb_info 
> > > *sbi,
> > >   set_cp(overprov_segment_count, 
> > > get_cp(rsvd_segment_count));
> > >   set_cp(overprov_segment_count, get_cp(overprov_segment_count) +
> > > - 2 * get_sb(segs_per_sec));
> > > + 2 * get_newsb(segs_per_sec));
> > >   DBG(0, "Info: Overprovision ratio = %.3lf%%\n", 
> > > c.new_overprovision);
> > >   DBG(0, "Info: Overprovision segments = %u (GC reserved = %u)\n",
> > > @@ -551,11 +551,12 @@ static void rebuild_checkpoint(struct f2fs_sb_info 
> > > *sbi,
> > >   
> > > cpu_to_le32(crc);
> > >   /* Write a new checkpoint in the other set */
> > > - new_cp_blk_no = old_cp_blk_no = get_sb(cp_blkaddr);
> > > + old_cp_blk_no = get_sb(cp_blkaddr);
> > > + new_cp_blk_no = get_newsb(cp_blkaddr);
> > >   if (sbi->cur_cp == 2)
> > >   old_cp_blk_no += 1 << get_sb(log_blocks_per_seg);
> > >   else
> > > - new_cp_blk_no += 1 << get_sb(log_blocks_per_seg);
> > > + new_cp_blk_no += 1 << get_newsb(log_blocks_per_seg);
> > >   /* write first cp */
> > >   ret = dev_write_block(new_cp, new_cp_blk_no++);
> > > -- 
> > > 2.40.1


___
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: don't set RO when shutting down f2fs

2024-04-09 Thread Daeho Jeong 
On Tue, Apr 9, 2024 at 9:21 AM Jaegeuk Kim  wrote:
>
> Shutdown does not check the error of thaw_super due to readonly, which
> causes a deadlock like below.
>
> f2fs_ioc_shutdown(F2FS_GOING_DOWN_FULLSYNC)issue_discard_thread
>  - bdev_freeze
>   - freeze_super
>  - f2fs_stop_checkpoint()
>   - f2fs_handle_critical_error - sb_start_write
> - set RO - waiting
>  - bdev_thaw
>   - thaw_super_locked
> - return -EINVAL, if sb_rdonly()
>  - f2fs_stop_discard_thread
>   -> wait for kthread_stop(discard_thread);
>
> Reported-by: "Light Hsieh (謝明燈)" 
> Signed-off-by: Jaegeuk Kim 
> ---
>
>  Change log from v1:
>   - use better variable
>   - fix typo
>
>  fs/f2fs/super.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 8ac4734c2df6..df32573d1f62 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -4159,9 +4159,15 @@ void f2fs_handle_critical_error(struct f2fs_sb_info 
> *sbi, unsigned char reason,
> if (shutdown)
> set_sbi_flag(sbi, SBI_IS_SHUTDOWN);
>
> -   /* continue filesystem operators if errors=continue */
> -   if (continue_fs || f2fs_readonly(sb))
> +   /*
> +* Continue filesystem operators if errors=continue. Should not set
> +* RO by shutdown, since RO bypasses thaw_super which can hang the
> +* system.
> +*/
> +   if (continue_fs || f2fs_readonly(sb) || shutdown) {
> +   f2fs_warn(sbi, "Stopped filesystem due to reason: %d", 
> reason);
> return;
> +   }
>
> f2fs_warn(sbi, "Remounting filesystem read-only");
> /*
> --
> 2.44.0.478.gd926399ef9-goog
>
>

Reviewed-by: Daeho Jeong 


>
> ___
> 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

[f2fs-dev] [PATCH] f2fs: write missing last sum blk of file pinning section

2024-04-09 Thread Daeho Jeong 
From: Daeho Jeong 

While do not allocating a new section in advance for file pinning area, I
missed that we should write the sum block for the last segment of a file
pinning section.

Fixes: 9703d69d9d15 ("f2fs: support file pinning for zoned devices")
Signed-off-by: Daeho Jeong 
---
 fs/f2fs/segment.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 4fd76e867e0a..6474b7338e81 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -3559,6 +3559,8 @@ int f2fs_allocate_data_block(struct f2fs_sb_info *sbi, 
struct page *page,
if (segment_full) {
if (type == CURSEG_COLD_DATA_PINNED &&
!((curseg->segno + 1) % sbi->segs_per_sec)) {
+   write_sum_page(sbi, curseg->sum_blk,
+   GET_SUM_BLOCK(sbi, curseg->segno));
reset_curseg_fields(curseg);
goto skip_new_segment;
}
-- 
2.44.0.478.gd926399ef9-goog



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

[f2fs-dev] [PATCH 3/3] f2fs: fix false alarm on invalid block address

2024-04-09 Thread Jaegeuk Kim 
f2fs_ra_meta_pages can try to read ahead on invalid block address which is
not the corruption case.

Fixes: 31f85ccc84b8 ("f2fs: unify the error handling of f2fs_is_valid_blkaddr")
Signed-off-by: Jaegeuk Kim 
---
 fs/f2fs/checkpoint.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index eac698b8dd38..b01320502624 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -179,22 +179,22 @@ static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info 
*sbi,
break;
case META_SIT:
if (unlikely(blkaddr >= SIT_BLK_CNT(sbi)))
-   goto err;
+   goto check_only;
break;
case META_SSA:
if (unlikely(blkaddr >= MAIN_BLKADDR(sbi) ||
blkaddr < SM_I(sbi)->ssa_blkaddr))
-   goto err;
+   goto check_only;
break;
case META_CP:
if (unlikely(blkaddr >= SIT_I(sbi)->sit_base_addr ||
blkaddr < __start_cp_addr(sbi)))
-   goto err;
+   goto check_only;
break;
case META_POR:
if (unlikely(blkaddr >= MAX_BLKADDR(sbi) ||
blkaddr < MAIN_BLKADDR(sbi)))
-   goto err;
+   goto check_only;
break;
case DATA_GENERIC:
case DATA_GENERIC_ENHANCE:
@@ -228,6 +228,7 @@ static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info 
*sbi,
return true;
 err:
f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
+check_only:
return false;
 }
 
-- 
2.44.0.478.gd926399ef9-goog



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

[f2fs-dev] [PATCH 1/3] f2fs: use folio_test_writeback

2024-04-09 Thread Jaegeuk Kim 
Let's convert PageWriteback to folio_test_writeback.

Signed-off-by: Jaegeuk Kim 
---
 fs/f2fs/compress.c |  2 +-
 fs/f2fs/data.c |  3 +--
 fs/f2fs/f2fs.h |  2 +-
 fs/f2fs/gc.c   |  2 +-
 fs/f2fs/inline.c   |  2 +-
 fs/f2fs/inode.c|  3 ++-
 fs/f2fs/node.c |  2 +-
 fs/f2fs/segment.c  | 10 +-
 8 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
index 8892c8262141..d67c471ab5df 100644
--- a/fs/f2fs/compress.c
+++ b/fs/f2fs/compress.c
@@ -1484,7 +1484,7 @@ static int f2fs_write_raw_pages(struct compress_ctx *cc,
if (!PageDirty(cc->rpages[i]))
goto continue_unlock;
 
-   if (PageWriteback(cc->rpages[i])) {
+   if (folio_test_writeback(page_folio(cc->rpages[i]))) {
if (wbc->sync_mode == WB_SYNC_NONE)
goto continue_unlock;
f2fs_wait_on_page_writeback(cc->rpages[i], DATA, true, 
true);
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 60056b9a51be..19f1e573297d 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -2707,8 +2707,7 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
if (err) {
if (fscrypt_inode_uses_fs_layer_crypto(inode))

fscrypt_finalize_bounce_page(>encrypted_page);
-   if (PageWriteback(page))
-   end_page_writeback(page);
+   end_page_writeback(page);
} else {
set_inode_flag(inode, FI_UPDATE_WRITE);
}
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index e9ef971f4dba..dd530dc70005 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -4660,7 +4660,7 @@ static inline void f2fs_truncate_meta_inode_pages(struct 
f2fs_sb_info *sbi,
 
page = find_get_page(META_MAPPING(sbi), blkaddr + i);
if (page) {
-   if (PageWriteback(page))
+   if (folio_test_writeback(page_folio(page)))
need_submit = true;
f2fs_put_page(page, 0);
}
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 8852814dab7f..ac4cbbe50c2f 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -1434,7 +1434,7 @@ static int move_data_page(struct inode *inode, block_t 
bidx, int gc_type,
goto out;
 
if (gc_type == BG_GC) {
-   if (PageWriteback(page)) {
+   if (folio_test_writeback(page_folio(page))) {
err = -EAGAIN;
goto out;
}
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index ac00423f117b..3d3218a4b29d 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -164,7 +164,7 @@ int f2fs_convert_inline_page(struct dnode_of_data *dn, 
struct page *page)
return -EFSCORRUPTED;
}
 
-   f2fs_bug_on(F2FS_P_SB(page), PageWriteback(page));
+   f2fs_bug_on(F2FS_P_SB(page), folio_test_writeback(page_folio(page)));
 
f2fs_do_read_inline_data(page, dn->inode_page);
set_page_dirty(page);
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index 12b1fef31f43..d7a5a88a1a5e 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -161,7 +161,8 @@ bool f2fs_inode_chksum_verify(struct f2fs_sb_info *sbi, 
struct page *page)
if (!f2fs_enable_inode_chksum(sbi, page))
 #else
if (!f2fs_enable_inode_chksum(sbi, page) ||
-   PageDirty(page) || PageWriteback(page))
+   PageDirty(page) ||
+   folio_test_writeback(page_folio(page)))
 #endif
return true;
 
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index bb57bbaff7b4..3b9eb5693683 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1743,7 +1743,7 @@ int f2fs_move_node_page(struct page *node_page, int 
gc_type)
goto release_page;
} else {
/* set page dirty and write it */
-   if (!PageWriteback(node_page))
+   if (!folio_test_writeback(page_folio(node_page)))
set_page_dirty(node_page);
}
 out_page:
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 4fd76e867e0a..065fd5919b48 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -3612,13 +3612,13 @@ int f2fs_allocate_data_block(struct f2fs_sb_info *sbi, 
struct page *page,
mutex_unlock(>curseg_mutex);
f2fs_up_read(_I(sbi)->curseg_lock);
return 0;
+
 out_err:
*new_blkaddr = NULL_ADDR;
up_write(_i->sentry_lock);
mutex_unlock(>curseg_mutex);
f2fs_up_read(_I(sbi)->curseg_lock);
return ret;
-
 }
 
 void f2fs_update_device_state(struct f2fs_sb_info *sbi, nid_t ino,
@@ -3660,8 +3660,7 @@ static void do_write_page(struct f2fs_summary *sum, 
struct f2fs_io_info *fio)
>new_blkaddr, sum, type, fio)) {

[f2fs-dev] [PATCH 2/3] f2fs: clear writeback when compression failed

2024-04-09 Thread Jaegeuk Kim 
Let's stop issuing compressed writes and clear their writeback flags.

Signed-off-by: Jaegeuk Kim 
---
 fs/f2fs/compress.c | 33 +++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
index d67c471ab5df..3a8ecc6aee84 100644
--- a/fs/f2fs/compress.c
+++ b/fs/f2fs/compress.c
@@ -1031,6 +1031,25 @@ static void set_cluster_writeback(struct compress_ctx 
*cc)
}
 }
 
+static void cancel_cluster_writeback(struct compress_ctx *cc, int submitted)
+{
+   int i;
+
+   for (i = 0; i < cc->cluster_size; i++) {
+   if (!cc->rpages[i])
+   continue;
+   if (i < submitted) {
+   if (i)
+   f2fs_wait_on_page_writeback(cc->rpages[i],
+   DATA, true, true);
+   inode_inc_dirty_pages(cc->inode);
+   lock_page(cc->rpages[i]);
+   }
+   clear_page_private_gcing(cc->rpages[i]);
+   end_page_writeback(cc->rpages[i]);
+   }
+}
+
 static void set_cluster_dirty(struct compress_ctx *cc)
 {
int i;
@@ -1232,7 +1251,6 @@ static int f2fs_write_compressed_pages(struct 
compress_ctx *cc,
.page = NULL,
.encrypted_page = NULL,
.compressed_page = NULL,
-   .submitted = 0,
.io_type = io_type,
.io_wbc = wbc,
.encrypted = fscrypt_inode_uses_fs_layer_crypto(cc->inode) ?
@@ -1358,7 +1376,15 @@ static int f2fs_write_compressed_pages(struct 
compress_ctx *cc,
fio.compressed_page = cc->cpages[i - 1];
 
cc->cpages[i - 1] = NULL;
+   fio.submitted = 0;
f2fs_outplace_write_data(, );
+   if (unlikely(!fio.submitted)) {
+   cancel_cluster_writeback(cc, i);
+
+   /* To call fscrypt_finalize_bounce_page */
+   i = cc->valid_nr_cpages;
+   goto out_destroy_crypt;
+   }
(*submitted)++;
 unlock_continue:
inode_dec_dirty_pages(cc->inode);
@@ -1392,8 +1418,11 @@ static int f2fs_write_compressed_pages(struct 
compress_ctx *cc,
 out_destroy_crypt:
page_array_free(cc->inode, cic->rpages, cc->cluster_size);
 
-   for (--i; i >= 0; i--)
+   for (--i; i >= 0; i--) {
+   if (!cc->cpages[i])
+   continue;
fscrypt_finalize_bounce_page(>cpages[i]);
+   }
 out_put_cic:
kmem_cache_free(cic_entry_slab, cic);
 out_put_dnode:
-- 
2.44.0.478.gd926399ef9-goog



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

Re: [f2fs-dev] [PATCH v3 01/13] fs: fiemap: add physical_length field to extents

2024-04-09 Thread Darrick J. Wong 
On Wed, Apr 03, 2024 at 03:22:42AM -0400, Sweet Tea Dorminy wrote:
> Some filesystems support compressed extents which have a larger logical
> size than physical, and for those filesystems, it can be useful for
> userspace to know how much space those extents actually use. For
> instance, the compsize [1] tool for btrfs currently uses btrfs-internal,
> root-only ioctl to find the actual disk space used by a file; it would
> be better and more useful for this information to require fewer
> privileges and to be usable on more filesystems. Therefore, use one of
> the padding u64s in the fiemap extent structure to return the actual
> physical length; and, for now, return this as equal to the logical
> length.
> 
> [1] https://github.com/kilobyte/compsize
> 
> Signed-off-by: Sweet Tea Dorminy 
> ---
>  Documentation/filesystems/fiemap.rst | 28 +---
>  fs/ioctl.c   |  3 ++-
>  include/uapi/linux/fiemap.h  | 32 ++--
>  3 files changed, 47 insertions(+), 16 deletions(-)
> 
> diff --git a/Documentation/filesystems/fiemap.rst 
> b/Documentation/filesystems/fiemap.rst
> index 93fc96f760aa..c2bfa107c8d7 100644
> --- a/Documentation/filesystems/fiemap.rst
> +++ b/Documentation/filesystems/fiemap.rst
> @@ -80,14 +80,24 @@ Each extent is described by a single fiemap_extent 
> structure as
>  returned in fm_extents::
>  
>  struct fiemap_extent {
> - __u64   fe_logical;  /* logical offset in bytes for the start of
> - * the extent */
> - __u64   fe_physical; /* physical offset in bytes for the start
> - * of the extent */
> - __u64   fe_length;   /* length in bytes for the extent */
> - __u64   fe_reserved64[2];
> - __u32   fe_flags;/* FIEMAP_EXTENT_* flags for this extent */
> - __u32   fe_reserved[3];
> +/*
> + * logical offset in bytes for the start of
> + * the extent from the beginning of the file
> + */
> +__u64 fe_logical;
> +/*
> + * physical offset in bytes for the start
> + * of the extent from the beginning of the disk
> + */
> +__u64 fe_physical;
> +/* logical length in bytes for this extent */
> +__u64 fe_logical_length;
> +/* physical length in bytes for this extent */
> +__u64 fe_physical_length;
> +__u64 fe_reserved64[1];
> +/* FIEMAP_EXTENT_* flags for this extent */
> +__u32 fe_flags;
> +__u32 fe_reserved[3];
>  };
>  
>  All offsets and lengths are in bytes and mirror those on disk.  It is valid
> @@ -175,6 +185,8 @@ FIEMAP_EXTENT_MERGED
>userspace would be highly inefficient, the kernel will try to merge most
>adjacent blocks into 'extents'.
>  
> +FIEMAP_EXTENT_HAS_PHYS_LEN
> +  This will be set if the file system populated the physical length field.

Just out of curiosity, should filesystems set this flag and
fe_physical_length if fe_physical_length == fe_logical_length?
Or just leave both blank?

>  VFS -> File System Implementation
>  -
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 661b46125669..8afd32e1a27a 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -138,7 +138,8 @@ int fiemap_fill_next_extent(struct fiemap_extent_info 
> *fieinfo, u64 logical,
>   memset(, 0, sizeof(extent));
>   extent.fe_logical = logical;
>   extent.fe_physical = phys;
> - extent.fe_length = len;
> + extent.fe_logical_length = len;
> + extent.fe_physical_length = len;
>   extent.fe_flags = flags;
>  
>   dest += fieinfo->fi_extents_mapped;
> diff --git a/include/uapi/linux/fiemap.h b/include/uapi/linux/fiemap.h
> index 24ca0c00cae3..3079159b8e94 100644
> --- a/include/uapi/linux/fiemap.h
> +++ b/include/uapi/linux/fiemap.h
> @@ -14,14 +14,30 @@
>  
>  #include 
>  
> +/*
> + * For backward compatibility, where the member of the struct was called
> + * fe_length instead of fe_logical_length.
> + */
> +#define fe_length fe_logical_length

This #define has global scope; are you sure this isn't going to cause a
weird build problem downstream with some program that declares an
unrelated fe_length symbol?

> +
>  struct fiemap_extent {
> - __u64 fe_logical;  /* logical offset in bytes for the start of
> - * the extent from the beginning of the file */
> - __u64 fe_physical; /* physical offset in bytes for the start
> - * of the extent from the beginning of the disk */
> - __u64 fe_length;   /* length in bytes for this extent */
> - __u64 fe_reserved64[2];
> - __u32 fe_flags;/* FIEMAP_EXTENT_* flags for this extent */
> + /*
> +  * logical offset in bytes for the start of
> +  * the extent from the beginning of the file
> +  */
> + __u64

Re: [f2fs-dev] [PATCH] f2fs: don't set RO when shutting down f2fs

2024-04-09 Thread Jaegeuk Kim 
On 04/09, Chao Yu wrote:
> On 2024/4/5 3:52, Jaegeuk Kim wrote:
> > Shutdown does not check the error of thaw_super due to readonly, which
> > causes a deadlock like below.
> > 
> > f2fs_ioc_shutdown(F2FS_GOING_DOWN_FULLSYNC)issue_discard_thread
> >   - bdev_freeze
> >- freeze_super
> >   - f2fs_stop_checkpoint()
> >- f2fs_handle_critical_error - sb_start_write
> >  - set RO - waiting
> >   - bdev_thaw
> >- thaw_super_locked
> >  - return -EINVAL, if sb_rdonly()
> >   - f2fs_stop_discard_thread
> >-> wait for kthread_stop(discard_thread);
> > 
> > Reported-by: "Light Hsieh (謝明燈)" 
> > Signed-off-by: Jaegeuk Kim 
> > ---
> >   fs/f2fs/super.c | 11 +--
> >   1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > index df9765b41dac..ba6288e870c5 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -4135,9 +4135,16 @@ void f2fs_handle_critical_error(struct f2fs_sb_info 
> > *sbi, unsigned char reason,
> > if (shutdown)
> > set_sbi_flag(sbi, SBI_IS_SHUTDOWN);
> > -   /* continue filesystem operators if errors=continue */
> > -   if (continue_fs || f2fs_readonly(sb))
> > +   /*
> > +* Continue filesystem operators if errors=continue. Should not set
> > +* RO by shutdown, since RO bypasses thaw_super which can hang the
> > +* system.
> > +*/
> > +   if (continue_fs || f2fs_readonly(sb) ||
> > +   reason == STOP_CP_REASON_SHUTDOWN) {
> > +   f2fs_warn(sbi, "Stopped filesystem due to readon: %d", reason);
> > return;
> 
> Do we need to set RO after bdev_thaw() in f2fs_do_shutdown()?

IIRC, shutdown doesn't need to set RO as we stopped the checkpoint.
I'm more concerned on any side effect caused by this RO change.

> 
> Thanks,
> 
> > +   }
> > f2fs_warn(sbi, "Remounting filesystem read-only");
> > /*


___
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: don't set RO when shutting down f2fs

2024-04-09 Thread Jaegeuk Kim 
Shutdown does not check the error of thaw_super due to readonly, which
causes a deadlock like below.

f2fs_ioc_shutdown(F2FS_GOING_DOWN_FULLSYNC)issue_discard_thread
 - bdev_freeze
  - freeze_super
 - f2fs_stop_checkpoint()
  - f2fs_handle_critical_error - sb_start_write
- set RO - waiting
 - bdev_thaw
  - thaw_super_locked
- return -EINVAL, if sb_rdonly()
 - f2fs_stop_discard_thread
  -> wait for kthread_stop(discard_thread);

Reported-by: "Light Hsieh (謝明燈)" 
Signed-off-by: Jaegeuk Kim 
---

 Change log from v1:
  - use better variable
  - fix typo

 fs/f2fs/super.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 8ac4734c2df6..df32573d1f62 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -4159,9 +4159,15 @@ void f2fs_handle_critical_error(struct f2fs_sb_info 
*sbi, unsigned char reason,
if (shutdown)
set_sbi_flag(sbi, SBI_IS_SHUTDOWN);
 
-   /* continue filesystem operators if errors=continue */
-   if (continue_fs || f2fs_readonly(sb))
+   /*
+* Continue filesystem operators if errors=continue. Should not set
+* RO by shutdown, since RO bypasses thaw_super which can hang the
+* system.
+*/
+   if (continue_fs || f2fs_readonly(sb) || shutdown) {
+   f2fs_warn(sbi, "Stopped filesystem due to reason: %d", reason);
return;
+   }
 
f2fs_warn(sbi, "Remounting filesystem read-only");
/*
-- 
2.44.0.478.gd926399ef9-goog



___
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 set RO when shutting down f2fs

2024-04-09 Thread Daeho Jeong 
On Thu, Apr 4, 2024 at 12:54 PM Jaegeuk Kim  wrote:
>
> Shutdown does not check the error of thaw_super due to readonly, which
> causes a deadlock like below.
>
> f2fs_ioc_shutdown(F2FS_GOING_DOWN_FULLSYNC)issue_discard_thread
>  - bdev_freeze
>   - freeze_super
>  - f2fs_stop_checkpoint()
>   - f2fs_handle_critical_error - sb_start_write
> - set RO - waiting
>  - bdev_thaw
>   - thaw_super_locked
> - return -EINVAL, if sb_rdonly()
>  - f2fs_stop_discard_thread
>   -> wait for kthread_stop(discard_thread);
>
> Reported-by: "Light Hsieh (謝明燈)" 
> Signed-off-by: Jaegeuk Kim 
> ---
>  fs/f2fs/super.c | 11 +--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index df9765b41dac..ba6288e870c5 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -4135,9 +4135,16 @@ void f2fs_handle_critical_error(struct f2fs_sb_info 
> *sbi, unsigned char reason,
> if (shutdown)
> set_sbi_flag(sbi, SBI_IS_SHUTDOWN);
>
> -   /* continue filesystem operators if errors=continue */
> -   if (continue_fs || f2fs_readonly(sb))
> +   /*
> +* Continue filesystem operators if errors=continue. Should not set
> +* RO by shutdown, since RO bypasses thaw_super which can hang the
> +* system.
> +*/
> +   if (continue_fs || f2fs_readonly(sb) ||
> +   reason == STOP_CP_REASON_SHUTDOWN) {

I think we can use "shutdown" variable instead of "reason ==
STOP_CP_REASON_SHUTDOWN" to be concise.

> +   f2fs_warn(sbi, "Stopped filesystem due to readon: %d", 
> reason);

readon -> reason?

> return;
> +   }
>
> f2fs_warn(sbi, "Remounting filesystem read-only");
> /*
> --
> 2.44.0.478.gd926399ef9-goog
>
>
>
> ___
> 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: Fix incorrect return value

2024-04-09 Thread wangjianjian (C) via Linux-f2fs-devel 

On 2024/4/7 14:23, Chao Yu wrote:

On 2024/4/4 21:47, Wang Jianjian wrote:

dquot_mark_dquot_dirty returns old dirty state not the error code.


I think it's fine to just pass return value of dquot_mark_dquot_dirty()
to caller, because caller can distinguish status from return value:
1) < 0, there is an error, 2) >= 0, there is no error, previously it is
dirty if it is 1.
mark_all_dquot_dirty uses if return value is 0 to save error code. It 
may cause mess.

By the way, I am fine don't change it.



Thanks,



Signed-off-by: Wang Jianjian 
---
  fs/f2fs/super.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index a6867f26f141..af07027475d9 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -3063,13 +3063,13 @@ static int f2fs_dquot_mark_dquot_dirty(struct 
dquot *dquot)

  {
  struct super_block *sb = dquot->dq_sb;
  struct f2fs_sb_info *sbi = F2FS_SB(sb);
-    int ret = dquot_mark_dquot_dirty(dquot);
+    dquot_mark_dquot_dirty(dquot);
  /* if we are using journalled quota */
  if (is_journalled_quota(sbi))
  set_sbi_flag(sbi, SBI_QUOTA_NEED_FLUSH);
-    return ret;
+    return 0;
  }
  static int f2fs_dquot_commit_info(struct super_block *sb, int type)



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

--
Regards



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