Re: [PATCH 06/12] xfs: remove XFS_TRANS_NOFS

2019-06-25 Thread Christoph Hellwig
On Tue, Jun 25, 2019 at 08:59:04AM +1000, Dave Chinner wrote:
> On Mon, Jun 24, 2019 at 07:52:47AM +0200, Christoph Hellwig wrote:
> > Instead of a magic flag for xfs_trans_alloc, just ensure all callers
> > that can't relclaim through the file system use memalloc_nofs_save to
> > set the per-task nofs flag.
> 
> I'm thinking that it would be a good idea to add comments to explain
> exactly what the memalloc_nofs_save/restore() are protecting where
> they are used. Right now the XFS_TRANS_NOFS flag is largely
> undocumented, so a reader is left guessing as to why the flag is
> necessary and what contexts it may apply to. Hence I think we should
> fix that while we are changing over to a different GFP_NOFS
> allocation context mechanism

Sure.


Re: [PATCH 06/12] xfs: remove XFS_TRANS_NOFS

2019-06-24 Thread Dave Chinner
On Mon, Jun 24, 2019 at 07:52:47AM +0200, Christoph Hellwig wrote:
> Instead of a magic flag for xfs_trans_alloc, just ensure all callers
> that can't relclaim through the file system use memalloc_nofs_save to
> set the per-task nofs flag.

I'm thinking that it would be a good idea to add comments to explain
exactly what the memalloc_nofs_save/restore() are protecting where
they are used. Right now the XFS_TRANS_NOFS flag is largely
undocumented, so a reader is left guessing as to why the flag is
necessary and what contexts it may apply to. Hence I think we should
fix that while we are changing over to a different GFP_NOFS
allocation context mechanism

Cheers,

Dave.
-- 
Dave Chinner
da...@fromorbit.com


Re: [PATCH 06/12] xfs: remove XFS_TRANS_NOFS

2019-06-24 Thread Darrick J. Wong
On Mon, Jun 24, 2019 at 07:52:47AM +0200, Christoph Hellwig wrote:
> Instead of a magic flag for xfs_trans_alloc, just ensure all callers
> that can't relclaim through the file system use memalloc_nofs_save to
> set the per-task nofs flag.
> 
> Signed-off-by: Christoph Hellwig 

Hmm this finally fixes up the mess I left where COW fork cleanup
sometimes needs nofs and other times doesn't... :)

> ---
>  fs/xfs/libxfs/xfs_shared.h |  1 -
>  fs/xfs/xfs_aops.c  | 12 +---
>  fs/xfs/xfs_file.c  | 12 +---
>  fs/xfs/xfs_iomap.c |  2 +-
>  fs/xfs/xfs_reflink.c   |  4 ++--
>  fs/xfs/xfs_trans.c |  4 +---
>  6 files changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
> index 4e909791aeac..1f2b5a0c71b4 100644
> --- a/fs/xfs/libxfs/xfs_shared.h
> +++ b/fs/xfs/libxfs/xfs_shared.h
> @@ -65,7 +65,6 @@ voidxfs_log_get_max_trans_res(struct xfs_mount *mp,
>  #define XFS_TRANS_DQ_DIRTY   0x10/* at least one dquot in trx dirty */
>  #define XFS_TRANS_RESERVE0x20/* OK to use reserved data blocks */
>  #define XFS_TRANS_NO_WRITECOUNT 0x40 /* do not elevate SB writecount */
> -#define XFS_TRANS_NOFS   0x80/* pass KM_NOFS to kmem_alloc */
>  /*
>   * LOWMODE is used by the allocator to activate the lowspace algorithm - when
>   * free space is running low the extent allocator may choose to allocate an
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index 93a760f13017..633baaaff7ae 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -138,8 +138,7 @@ xfs_setfilesize_trans_alloc(
>   struct xfs_trans*tp;
>   int error;
>  
> - error = xfs_trans_alloc(mp, &M_RES(mp)->tr_fsyncts, 0, 0,
> - XFS_TRANS_NOFS, &tp);
> + error = xfs_trans_alloc(mp, &M_RES(mp)->tr_fsyncts, 0, 0, 0, &tp);
>   if (error)
>   return error;
>  
> @@ -236,6 +235,7 @@ STATIC void
>  xfs_end_ioend(
>   struct xfs_ioend*ioend)
>  {
> + unsigned intnofs_flag = memalloc_nofs_save();
>   struct list_headioend_list;
>   struct xfs_inode*ip = XFS_I(ioend->io_inode);
>   xfs_off_t   offset = ioend->io_offset;
> @@ -282,6 +282,8 @@ xfs_end_ioend(
>   list_del_init(&ioend->io_list);
>   xfs_destroy_ioend(ioend, error);
>   }
> +
> + memalloc_nofs_restore(nofs_flag);
>  }
>  
>  /*
> @@ -663,8 +665,12 @@ xfs_submit_ioend(
>   (ioend->io_fork == XFS_COW_FORK ||
>ioend->io_type != IOMAP_UNWRITTEN) &&
>   xfs_ioend_is_append(ioend) &&
> - !ioend->io_append_trans)
> + !ioend->io_append_trans) {
> + unsigned nofs_flag = memalloc_nofs_save();

unsigned int?  Seeing as you use that everywhere else...

--D

> +
>   status = xfs_setfilesize_trans_alloc(ioend);
> + memalloc_nofs_restore(nofs_flag);
> + }
>  
>   ioend->io_bio->bi_private = ioend;
>   ioend->io_bio->bi_end_io = xfs_end_bio;
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 916a35cae5e9..f2d806ef8f06 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -379,6 +379,7 @@ xfs_dio_write_end_io(
>   struct inode*inode = file_inode(iocb->ki_filp);
>   struct xfs_inode*ip = XFS_I(inode);
>   loff_t  offset = iocb->ki_pos;
> + unsigned intnofs_flag;
>   int error = 0;
>  
>   trace_xfs_end_io_direct_write(ip, offset, size);
> @@ -395,10 +396,11 @@ xfs_dio_write_end_io(
>*/
>   XFS_STATS_ADD(ip->i_mount, xs_write_bytes, size);
>  
> + nofs_flag = memalloc_nofs_save();
>   if (flags & IOMAP_DIO_COW) {
>   error = xfs_reflink_end_cow(ip, offset, size);
>   if (error)
> - return error;
> + goto out;
>   }
>  
>   /*
> @@ -407,8 +409,10 @@ xfs_dio_write_end_io(
>* earlier allows a racing dio read to find unwritten extents before
>* they are converted.
>*/
> - if (flags & IOMAP_DIO_UNWRITTEN)
> - return xfs_iomap_write_unwritten(ip, offset, size, true);
> + if (flags & IOMAP_DIO_UNWRITTEN) {
> + error = xfs_iomap_write_unwritten(ip, offset, size, true);
> + goto out;
> + }
>  
>   /*
>* We need to update the in-core inode size here so that we don't end up
> @@ -430,6 +434,8 @@ xfs_dio_write_end_io(
>   spin_unlock(&ip->i_flags_lock);
>   }
>  
> +out:
> + memalloc_nofs_restore(nofs_flag);
>   return error;
>  }
>  
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 6b29452bfba0..461ea023b910 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -782,7 +782,7 @@ xfs_iomap_write_unwritten(
>* complete here and might deadlock on the iolock.
>