Re: [Cluster-devel] [PATCH 7/9] gfs2: handle a NULL folio in gfs2_jhead_process_page
On Wed, Jan 18, 2023 at 05:24:32PM +0100, Andreas Gruenbacher wrote: > We're actually still holding a reference to the folio from the > find_or_create_page() in gfs2_find_jhead() here, so we know that > memory pressure can't push the page out and filemap_get_folio() won't > return NULL. Ok, I'll drop this patch.
Re: [Cluster-devel] [PATCH 7/9] gfs2: handle a NULL folio in gfs2_jhead_process_page
[Christoph's email ended up in my spam folder; I hope that was a one-time-only occurrence.] On Wed, Jan 18, 2023 at 5:00 PM Matthew Wilcox wrote: > On Wed, Jan 18, 2023 at 10:43:27AM +0100, Christoph Hellwig wrote: > > filemap_get_folio can return NULL, so exit early for that case. > > I'm not sure it can return NULL in this specific case. As I understand > this code, we're scanning the journal looking for the log head. We've > just submitted the bio to read this page. I suppose memory pressure > could theoretically push the page out, but if it does, we're doing the > wrong thing by just returning here; we need to retry reading the page. > > Assuming we're not willing to do the work to add that case, I think I'd > rather see the crash in folio_wait_locked() than get data corruption > from failing to find the head of the log. > > > Signed-off-by: Christoph Hellwig > > --- > > fs/gfs2/lops.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c > > index 1902413d5d123e..51d4b610127cdb 100644 > > --- a/fs/gfs2/lops.c > > +++ b/fs/gfs2/lops.c > > @@ -472,6 +472,8 @@ static void gfs2_jhead_process_page(struct gfs2_jdesc > > *jd, unsigned long index, > > struct folio *folio; > > > > folio = filemap_get_folio(jd->jd_inode->i_mapping, index); > > + if (!folio) > > + return; We're actually still holding a reference to the folio from the find_or_create_page() in gfs2_find_jhead() here, so we know that memory pressure can't push the page out and filemap_get_folio() won't return NULL. > > > > folio_wait_locked(folio); > > if (folio_test_error(folio)) > > -- > > 2.39.0 > > > Thanks, Andreas
Re: [Cluster-devel] [PATCH 7/9] gfs2: handle a NULL folio in gfs2_jhead_process_page
On Wed, Jan 18, 2023 at 10:43:27AM +0100, Christoph Hellwig wrote: > filemap_get_folio can return NULL, so exit early for that case. I'm not sure it can return NULL in this specific case. As I understand this code, we're scanning the journal looking for the log head. We've just submitted the bio to read this page. I suppose memory pressure could theoretically push the page out, but if it does, we're doing the wrong thing by just returning here; we need to retry reading the page. Assuming we're not willing to do the work to add that case, I think I'd rather see the crash in folio_wait_locked() than get data corruption from failing to find the head of the log. > Signed-off-by: Christoph Hellwig > --- > fs/gfs2/lops.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c > index 1902413d5d123e..51d4b610127cdb 100644 > --- a/fs/gfs2/lops.c > +++ b/fs/gfs2/lops.c > @@ -472,6 +472,8 @@ static void gfs2_jhead_process_page(struct gfs2_jdesc > *jd, unsigned long index, > struct folio *folio; > > folio = filemap_get_folio(jd->jd_inode->i_mapping, index); > + if (!folio) > + return; > > folio_wait_locked(folio); > if (folio_test_error(folio)) > -- > 2.39.0 >
