On Mon 06-02-17 07:39:23, Matthew Wilcox wrote:
> On Mon, Feb 06, 2017 at 03:07:16PM +0100, Michal Hocko wrote:
> > +++ b/fs/xfs/xfs_buf.c
> > @@ -442,17 +442,17 @@ _xfs_buf_map_pages(
> > bp->b_addr = NULL;
> > } else {
> > int retried = 0;
> > - unsigned noio_flag;
> > + unsigned nofs_flag;
> >
> > /*
> > * vm_map_ram() will allocate auxillary structures (e.g.
> > * pagetables) with GFP_KERNEL, yet we are likely to be under
> > * GFP_NOFS context here. Hence we need to tell memory reclaim
> > - * that we are in such a context via PF_MEMALLOC_NOIO to prevent
> > + * that we are in such a context via PF_MEMALLOC_NOFS to prevent
> > * memory reclaim re-entering the filesystem here and
> > * potentially deadlocking.
> > */
>
> This comment feels out of date ... how about:
which part is out of date?
>
> /*
> * vm_map_ram will allocate auxiliary structures (eg page
> * tables) with GFP_KERNEL. If that tries to reclaim memory
> * by calling back into this filesystem, we may deadlock.
> * Prevent that by setting the NOFS flag.
> */
dunno, the previous wording seems clear enough to me. Maybe little bit
more chatty than yours but I am not sure this is worth changing.
>
> > - noio_flag = memalloc_noio_save();
> > + nofs_flag = memalloc_nofs_save();
> > do {
> > bp->b_addr = vm_map_ram(bp->b_pages, bp->b_page_count,
> > -1, PAGE_KERNEL);
>
> Also, I think it shows that this is the wrong place in XFS to be calling
> memalloc_nofs_save(). I'm not arguing against including this patch;
> it's a step towards where we want to be. I also don't know XFS well
> enough to know where to set that flag ;-) Presumably when we start a
> transaction ... ?
Yes that is what I would like to achieve longterm. And the reason why I
didn't want to mimic this pattern in kvmalloc as some have suggested.
It just takes much more time to get there from the past experience and
we should really start somewhere.
--
Michal Hocko
SUSE Labs
