| Message ID | 20250106033645.4043618-1-zangyangyang1@xiaomi.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [f2fs-dev,v3] f2fs: fix using wrong 'submitted' value in f2fs_write_cache_pages | expand |
On 01/06, zangyangyang wrote: > From: zangyangyang1 <zangyangyang1@xiaomi.com> > > When f2fs_write_single_data_page fails, f2fs_write_cache_pages > will use the last 'submitted' value incorrectly, which will cause > 'nwritten' and 'wbc->nr_to_write' calculation errors > > Signed-off-by: zangyangyang1 <zangyangyang1@xiaomi.com> > --- > v3: No logical changes, just format patch > v2: Initialize "submitted" in f2fs_write_single_data_page() > --- > fs/f2fs/data.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index 94f7b084f601..f772fbc7f331 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -2816,6 +2816,9 @@ int f2fs_write_single_data_page(struct folio *folio, int *submitted, > > trace_f2fs_writepage(folio, DATA); > > + if (submitted) > + *submitted = 0; I don't think this is correct, as callers should handle it. > + > /* we should bypass data pages to proceed the kworker jobs */ > if (unlikely(f2fs_cp_error(sbi))) { > mapping_set_error(folio->mapping, -EIO); > -- > 2.43.2
Jaegeuk Kim <jaegeuk@kernel.org> 于2025年1月9日周四 02:29写道： > > On 01/06, zangyangyang wrote: > > From: zangyangyang1 <zangyangyang1@xiaomi.com> > > > > When f2fs_write_single_data_page fails, f2fs_write_cache_pages > > will use the last 'submitted' value incorrectly, which will cause > > 'nwritten' and 'wbc->nr_to_write' calculation errors > > > > Signed-off-by: zangyangyang1 <zangyangyang1@xiaomi.com> > > --- > > v3: No logical changes, just format patch > > v2: Initialize "submitted" in f2fs_write_single_data_page() > > --- > > fs/f2fs/data.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > > index 94f7b084f601..f772fbc7f331 100644 > > --- a/fs/f2fs/data.c > > +++ b/fs/f2fs/data.c > > @@ -2816,6 +2816,9 @@ int f2fs_write_single_data_page(struct folio *folio, int *submitted, > > > > trace_f2fs_writepage(folio, DATA); > > > > + if (submitted) > > + *submitted = 0; > > I don't think this is correct, as callers should handle it. Hi, Chao, what do you think? Thanks > > > + > > /* we should bypass data pages to proceed the kworker jobs */ > > if (unlikely(f2fs_cp_error(sbi))) { > > mapping_set_error(folio->mapping, -EIO); > > -- > > 2.43.2
On 1/9/25 10:23, 臧阳阳 wrote: > Jaegeuk Kim <jaegeuk@kernel.org> 于2025年1月9日周四 02:29写道： > >> >> On 01/06, zangyangyang wrote: >>> From: zangyangyang1 <zangyangyang1@xiaomi.com> >>> >>> When f2fs_write_single_data_page fails, f2fs_write_cache_pages >>> will use the last 'submitted' value incorrectly, which will cause >>> 'nwritten' and 'wbc->nr_to_write' calculation errors >>> >>> Signed-off-by: zangyangyang1 <zangyangyang1@xiaomi.com> >>> --- >>> v3: No logical changes, just format patch >>> v2: Initialize "submitted" in f2fs_write_single_data_page() >>> --- >>> fs/f2fs/data.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c >>> index 94f7b084f601..f772fbc7f331 100644 >>> --- a/fs/f2fs/data.c >>> +++ b/fs/f2fs/data.c >>> @@ -2816,6 +2816,9 @@ int f2fs_write_single_data_page(struct folio *folio, int *submitted, >>> >>> trace_f2fs_writepage(folio, DATA); >>> >>> + if (submitted) >>> + *submitted = 0; >> >> I don't think this is correct, as callers should handle it. > > Hi, Chao, what do you think? Both are fine to me, previously I suggest to handle it in f2fs_write_single_data_page(), because I'm worried about we may miss to handle it in any possible caller in further. Thanks, > > Thanks > >> >>> + >>> /* we should bypass data pages to proceed the kworker jobs */ >>> if (unlikely(f2fs_cp_error(sbi))) { >>> mapping_set_error(folio->mapping, -EIO); >>> -- >>> 2.43.2
On Thu, Jan 9, 2025 at 6:19 PM Chao Yu <chao@kernel.org> wrote: > > On 1/9/25 10:23, 臧阳阳 wrote: > > Jaegeuk Kim <jaegeuk@kernel.org> 于2025年1月9日周四 02:29写道： > > > >> > >> On 01/06, zangyangyang wrote: > >>> From: zangyangyang1 <zangyangyang1@xiaomi.com> > >>> > >>> When f2fs_write_single_data_page fails, f2fs_write_cache_pages > >>> will use the last 'submitted' value incorrectly, which will cause > >>> 'nwritten' and 'wbc->nr_to_write' calculation errors > >>> > >>> Signed-off-by: zangyangyang1 <zangyangyang1@xiaomi.com> > >>> --- > >>> v3: No logical changes, just format patch > >>> v2: Initialize "submitted" in f2fs_write_single_data_page() > >>> --- > >>> fs/f2fs/data.c | 3 +++ > >>> 1 file changed, 3 insertions(+) > >>> > >>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > >>> index 94f7b084f601..f772fbc7f331 100644 > >>> --- a/fs/f2fs/data.c > >>> +++ b/fs/f2fs/data.c > >>> @@ -2816,6 +2816,9 @@ int f2fs_write_single_data_page(struct folio *folio, int *submitted, > >>> > >>> trace_f2fs_writepage(folio, DATA); > >>> > >>> + if (submitted) > >>> + *submitted = 0; > >> > >> I don't think this is correct, as callers should handle it. > > > > Hi, Chao, what do you think? > > Both are fine to me, previously I suggest to handle it in > f2fs_write_single_data_page(), because I'm worried about we may miss to handle > it in any possible caller in further. Thank you very much, Chao. Hi, Kim, If you still think callers should handle it, I'll send a new patch. Thanks, > > Thanks, > > > > > Thanks > > > >> > >>> + > >>> /* we should bypass data pages to proceed the kworker jobs */ > >>> if (unlikely(f2fs_cp_error(sbi))) { > >>> mapping_set_error(folio->mapping, -EIO); > >>> -- > >>> 2.43.2 >
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index 94f7b084f601..f772fbc7f331 100644 --- a/fs/f2fs/data.c +++ b/fs/f2fs/data.c @@ -2816,6 +2816,9 @@ int f2fs_write_single_data_page(struct folio *folio, int *submitted, trace_f2fs_writepage(folio, DATA); + if (submitted) + *submitted = 0; + /* we should bypass data pages to proceed the kworker jobs */ if (unlikely(f2fs_cp_error(sbi))) { mapping_set_error(folio->mapping, -EIO);