On Wed, 16 Apr 2014 16:04:59 +1000 Dave Chinner <da...@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 <ne...@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
signature.asc
Description: PGP signature
