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

[Joanne Koong <joannelkoong@gmail.com>]

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).

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