Re: [PATCH v4 1/6] mm: add AS_WRITEBACK_MAY_BLOCK mapping flag

[Joanne Koong <joannelkoong@gmail.com>]

On Fri, Nov 15, 2024 at 11:33 AM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> On Mon, Nov 11, 2024 at 1:11 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> >
> > On Fri, Nov 8, 2024 at 4:10 PM Shakeel Butt <shakeel.butt@linux.dev> wrote:
> > >
> > > On Thu, Nov 07, 2024 at 03:56:09PM -0800, Joanne Koong wrote:
> > > > Add a new mapping flag AS_WRITEBACK_MAY_BLOCK which filesystems may set
> > > > to indicate that writeback operations may block or take an indeterminate
> > > > amount of time to complete. Extra caution should be taken when waiting
> > > > on writeback for folios belonging to mappings where this flag is set.
> > > >
> > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > > ---
> > > >  include/linux/pagemap.h | 11 +++++++++++
> > > >  1 file changed, 11 insertions(+)
> > > >
> > > > diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> > > > index 68a5f1ff3301..eb5a7837e142 100644
> > > > --- a/include/linux/pagemap.h
> > > > +++ b/include/linux/pagemap.h
> > > > @@ -210,6 +210,7 @@ enum mapping_flags {
> > > >       AS_STABLE_WRITES = 7,   /* must wait for writeback before modifying
> > > >                                  folio contents */
> > > >       AS_INACCESSIBLE = 8,    /* Do not attempt direct R/W access to the mapping */
> > > > +     AS_WRITEBACK_MAY_BLOCK = 9, /* Use caution when waiting on writeback */
> > >
> > > To me 'may block' does not feel right. For example in reclaim code,
> > > folio_wait_writeback() can get blocked and that is fine. However with
> > > non-privileged fuse involved, there are security concerns. Somehow 'may
> > > block' does not convey that. Anyways, I am not really pushing back but
> > > I think there is a need for better name here.
> >
> > Ahh I see where this naming causes confusion - the "MAY_BLOCK" part
> > could be interpreted in two ways: a) may block as in it's possible for
> > the writeback to block and b) may block as in it's permissible/ok for
> > the writeback to block. I intended "may block" to signify a) but
> > you're right, it could be easily interpreted as b).
> >
> > I'll change this to AS_WRITEBACK_BLOCKING.
>
> Thinking about this some more, I think AS_WRITEBACK_ASYNC would be a
> better name. (AS_WRITEBACK_BLOCKING might imply that the writeback
> ->writepages() operation itself is blocking).

Ugh, AS_WRITEBACK_ASYNC probably doesn't work either since NFS is also
async. Okay, maybe "AS_WRITEBACK_INDETERMINATE" then? We can keep
riffing on this, for v5 I'll submit it using
AS_WRITEBACK_INDETERMINATE.

>
> I'll make this change for v5.
>
> Thanks,
> Joanne
>
> >
> > Thanks,
> > Joanne
> >
> > >
> > > >       /* Bits 16-25 are used for FOLIO_ORDER */
> > > >       AS_FOLIO_ORDER_BITS = 5,
> > > >       AS_FOLIO_ORDER_MIN = 16,
> > > > @@ -335,6 +336,16 @@ static inline bool mapping_inaccessible(struct address_space *mapping)
> > > >       return test_bit(AS_INACCESSIBLE, &mapping->flags);
> > > >  }
> > > >
> > > > +static inline void mapping_set_writeback_may_block(struct address_space *mapping)
> > > > +{
> > > > +     set_bit(AS_WRITEBACK_MAY_BLOCK, &mapping->flags);
> > > > +}
> > > > +
> > > > +static inline bool mapping_writeback_may_block(struct address_space *mapping)
> > > > +{
> > > > +     return test_bit(AS_WRITEBACK_MAY_BLOCK, &mapping->flags);
> > > > +}
> > > > +
> > > >  static inline gfp_t mapping_gfp_mask(struct address_space * mapping)
> > > >  {
> > > >       return mapping->gfp_mask;
> > > > --
> > > > 2.43.5
> > > >