Please drop '03e02b94171b1985dd0aa184296fe94425b855a3' fs: nfs: fix missing refcnt by replacing folio_set_private by folio_attach_private

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Anna and Trond,
Please drop '03e02b94171b1985dd0aa184296fe94425b855a3' fs: nfs: fix
missing refcnt by replacing folio_set_private by folio_attach_private
since it is wrong as Trond has explained. Thank you!

On Tue, Sep 3, 2024 at 2:00 AM Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote:
>
> On Mon, 2024-09-02 at 10:42 +0800, Zhaoyang Huang wrote:
> > Hi Trond,
> > I am not sure if 'trondmy@xxxxxxxxxx' is still available or if you
> > are
> > too busy to respond. Try these two addresses.
> >
>
> I believe that I already answered this on the list. The refcount that
> this change adds is redundant because the struct nfs_page already holds
> its own refcount to the same folio.
>
> Thanks!
>
> > ---------- Forwarded message ---------
> > From: zhaoyang.huang <zhaoyang.huang@xxxxxxxxxx>
> > Date: Fri, Aug 30, 2024 at 11:28 AM
> > Subject: [PATCH 1/1] fs: nfs: fix missing refcnt by replacing
> > folio_set_private by folio_attach_private
> > To: Trond Myklebust <trondmy@xxxxxxxxxx>, Anna Schumaker
> > <anna@xxxxxxxxxx>, Christoph Hellwig <hch@xxxxxxxxxxxxx>,
> > <linux-nfs@xxxxxxxxxxxxxxx>, <linux-kernel@xxxxxxxxxxxxxxx>, Zhaoyang
> > Huang <huangzhaoyang@xxxxxxxxx>, <steve.kang@xxxxxxxxxx>
> >
> >
> > From: Zhaoyang Huang <zhaoyang.huang@xxxxxxxxxx>
> >
> > This patch is inspired by a code review of fs codes which aims at
> > folio's extra refcnt that could introduce unwanted behavious when
> > judging refcnt, such as[1].That is, the folio passed to
> > mapping_evict_folio carries the refcnts from find_lock_entries,
> > page_cache, corresponding to PTEs and folio's private if has.
> > However,
> > current code doesn't take the refcnt for folio's private which could
> > have mapping_evict_folio miss the one to only PTE and lead to
> > call filemap_release_folio wrongly.
> >
> > [1]
> > long mapping_evict_folio(struct address_space *mapping, struct folio
> > *folio)
> > {
> > ...
> > //current code will misjudge here if there is one pte on the folio
> > which
> > is be deemed as the one as folio's private
> >         if (folio_ref_count(folio) >
> >                         folio_nr_pages(folio) +
> > folio_has_private(folio) + 1)
> >                 return 0;
> >         if (!filemap_release_folio(folio, 0))
> >                 return 0;
> >
> >         return remove_mapping(mapping, folio);
> > }
> >
> > Signed-off-by: Zhaoyang Huang <zhaoyang.huang@xxxxxxxxxx>
> > ---
> >  fs/nfs/write.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> > index d074d0ceb4f0..80c6ded5f74c 100644
> > --- a/fs/nfs/write.c
> > +++ b/fs/nfs/write.c
> > @@ -772,8 +772,7 @@ static void nfs_inode_add_request(struct nfs_page
> > *req)
> >         nfs_lock_request(req);
> >         spin_lock(&mapping->i_private_lock);
> >         set_bit(PG_MAPPED, &req->wb_flags);
> > -       folio_set_private(folio);
> > -       folio->private = req;
> > +       folio_attach_private(folio, req);
> >         spin_unlock(&mapping->i_private_lock);
> >         atomic_long_inc(&nfsi->nrequests);
> >         /* this a head request for a page group - mark it as having
> > an
> > @@ -797,8 +796,7 @@ static void nfs_inode_remove_request(struct
> > nfs_page *req)
> >
> >                 spin_lock(&mapping->i_private_lock);
> >                 if (likely(folio)) {
> > -                       folio->private = NULL;
> > -                       folio_clear_private(folio);
> > +                       folio_detach_private(folio);
> >                         clear_bit(PG_MAPPED, &req->wb_head-
> > >wb_flags);
> >                 }
> >                 spin_unlock(&mapping->i_private_lock);
> > --
> > 2.25.1
>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@xxxxxxxxxxxxxxx
>
>





[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux