| Message ID | 20140416040336.10604.90380.stgit@notabene.brown (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 16, 2014 at 02:03:36PM +1000, NeilBrown wrote: > xfs_file_*_read holds an inode lock while calling a generic 'read' > function. These functions perform read-ahead and are quite likely to > allocate memory. Yes, that's what reading data from disk requires. > So set PF_FSTRANS to ensure they avoid __GFP_FS and so don't recurse > into a filesystem to free memory. We already have that protection via the > > This can be a problem with loop-back NFS mounts, if free_pages ends up > wating in nfs_release_page(), and nfsd is blocked waiting for the lock > that this code holds. > > This was found both by lockdep and as a real deadlock during testing. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/xfs/xfs_file.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > index 64b48eade91d..88b33ef64668 100644 > --- a/fs/xfs/xfs_file.c > +++ b/fs/xfs/xfs_file.c > @@ -243,6 +243,7 @@ xfs_file_aio_read( > ssize_t ret = 0; > int ioflags = 0; > xfs_fsize_t n; > + unsigned int pflags; > > XFS_STATS_INC(xs_read_calls); > > @@ -290,6 +291,10 @@ xfs_file_aio_read( > * proceeed concurrently without serialisation. > */ > xfs_rw_ilock(ip, XFS_IOLOCK_SHARED); > + /* As we hold a lock, we must ensure that any allocation > + * in generic_file_aio_read avoid __GFP_FS > + */ > + current_set_flags_nested(&pflags, PF_FSTRANS); Ugh. No. This is Simply Wrong. We handle the memory allocations in the IO path with GFP_NOFS/KM_NOFS where necessary. We also do this when setting up regular file inodes in xfs_setup_inode(): /* * Ensure all page cache allocations are done from GFP_NOFS context to * prevent direct reclaim recursion back into the filesystem and blowing * stacks or deadlocking. */ gfp_mask = mapping_gfp_mask(inode->i_mapping); mapping_set_gfp_mask(inode->i_mapping, (gfp_mask & ~(__GFP_FS))); Which handles all of the mapping allocations that occur within the page cache read/write paths. Remember, you removed the KM_NOFS code from the XFS allocator that caused it to clear __GFP_FS in an earlier patch - the read Io path is one of the things you broke by doing that.... If there are places where we don't use GFP_NOFS context allocations that we should, then we need to fix them individually.... Cheers, Dave.
On Wed, 16 Apr 2014 16:04:59 +1000 Dave Chinner <david@fromorbit.com> wrote: > On Wed, Apr 16, 2014 at 02:03:36PM +1000, NeilBrown wrote: > > xfs_file_*_read holds an inode lock while calling a generic 'read' > > function. These functions perform read-ahead and are quite likely to > > allocate memory. > > Yes, that's what reading data from disk requires. > > > So set PF_FSTRANS to ensure they avoid __GFP_FS and so don't recurse > > into a filesystem to free memory. > > We already have that protection via the > > > > This can be a problem with loop-back NFS mounts, if free_pages ends up > > wating in nfs_release_page(), and nfsd is blocked waiting for the lock > > that this code holds. > > > > This was found both by lockdep and as a real deadlock during testing. > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > --- > > fs/xfs/xfs_file.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > > index 64b48eade91d..88b33ef64668 100644 > > --- a/fs/xfs/xfs_file.c > > +++ b/fs/xfs/xfs_file.c > > @@ -243,6 +243,7 @@ xfs_file_aio_read( > > ssize_t ret = 0; > > int ioflags = 0; > > xfs_fsize_t n; > > + unsigned int pflags; > > > > XFS_STATS_INC(xs_read_calls); > > > > @@ -290,6 +291,10 @@ xfs_file_aio_read( > > * proceeed concurrently without serialisation. > > */ > > xfs_rw_ilock(ip, XFS_IOLOCK_SHARED); > > + /* As we hold a lock, we must ensure that any allocation > > + * in generic_file_aio_read avoid __GFP_FS > > + */ > > + current_set_flags_nested(&pflags, PF_FSTRANS); > > Ugh. No. This is Simply Wrong. > > We handle the memory allocations in the IO path with > GFP_NOFS/KM_NOFS where necessary. > > We also do this when setting up regular file inodes in > xfs_setup_inode(): > > /* > * Ensure all page cache allocations are done from GFP_NOFS context to > * prevent direct reclaim recursion back into the filesystem and blowing > * stacks or deadlocking. > */ > gfp_mask = mapping_gfp_mask(inode->i_mapping); > mapping_set_gfp_mask(inode->i_mapping, (gfp_mask & ~(__GFP_FS))); > > Which handles all of the mapping allocations that occur within the > page cache read/write paths. > > Remember, you removed the KM_NOFS code from the XFS allocator that > caused it to clear __GFP_FS in an earlier patch - the read Io path > is one of the things you broke by doing that.... > > If there are places where we don't use GFP_NOFS context allocations > that we should, then we need to fix them individually.... > > Cheers, > > Dave. Thanks Dave. Having fixed the KM_NOFS error I'll start retesting with all the other xfs patches removed. NeilBrown
On Wed, Apr 16, 2014 at 04:04:59PM +1000, Dave Chinner wrote: > On Wed, Apr 16, 2014 at 02:03:36PM +1000, NeilBrown wrote: > > xfs_file_*_read holds an inode lock while calling a generic 'read' > > function. These functions perform read-ahead and are quite likely to > > allocate memory. > > Yes, that's what reading data from disk requires. > > > So set PF_FSTRANS to ensure they avoid __GFP_FS and so don't recurse > > into a filesystem to free memory. > > We already have that protection via the Oops, stray paste. Ignore that comment. -Dave.
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index 64b48eade91d..88b33ef64668 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -243,6 +243,7 @@ xfs_file_aio_read( ssize_t ret = 0; int ioflags = 0; xfs_fsize_t n; + unsigned int pflags; XFS_STATS_INC(xs_read_calls); @@ -290,6 +291,10 @@ xfs_file_aio_read( * proceeed concurrently without serialisation. */ xfs_rw_ilock(ip, XFS_IOLOCK_SHARED); + /* As we hold a lock, we must ensure that any allocation + * in generic_file_aio_read avoid __GFP_FS + */ + current_set_flags_nested(&pflags, PF_FSTRANS); if ((ioflags & IO_ISDIRECT) && inode->i_mapping->nrpages) { xfs_rw_iunlock(ip, XFS_IOLOCK_SHARED); xfs_rw_ilock(ip, XFS_IOLOCK_EXCL); @@ -313,6 +318,7 @@ xfs_file_aio_read( if (ret > 0) XFS_STATS_ADD(xs_read_bytes, ret); + current_restore_flags_nested(&pflags, PF_FSTRANS); xfs_rw_iunlock(ip, XFS_IOLOCK_SHARED); return ret; } @@ -328,6 +334,7 @@ xfs_file_splice_read( struct xfs_inode *ip = XFS_I(infilp->f_mapping->host); int ioflags = 0; ssize_t ret; + unsigned int pflags; XFS_STATS_INC(xs_read_calls); @@ -338,6 +345,10 @@ xfs_file_splice_read( return -EIO; xfs_rw_ilock(ip, XFS_IOLOCK_SHARED); + /* As we hold a lock, we must ensure that any allocation + * in generic_file_splice_read avoid __GFP_FS + */ + current_set_flags_nested(&pflags, PF_FSTRANS); trace_xfs_file_splice_read(ip, count, *ppos, ioflags); @@ -345,6 +356,7 @@ xfs_file_splice_read( if (ret > 0) XFS_STATS_ADD(xs_read_bytes, ret); + current_restore_flags_nested(&pflags, PF_FSTRANS); xfs_rw_iunlock(ip, XFS_IOLOCK_SHARED); return ret; }
xfs_file_*_read holds an inode lock while calling a generic 'read' function. These functions perform read-ahead and are quite likely to allocate memory. So set PF_FSTRANS to ensure they avoid __GFP_FS and so don't recurse into a filesystem to free memory. This can be a problem with loop-back NFS mounts, if free_pages ends up wating in nfs_release_page(), and nfsd is blocked waiting for the lock that this code holds. This was found both by lockdep and as a real deadlock during testing. Signed-off-by: NeilBrown <neilb@suse.de> --- fs/xfs/xfs_file.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html