* [PATCH] xfs: lock bitmap/summary inodes in xfs_rtbuf_get() @ 2016-01-30 3:26 Eric Sandeen 2016-01-30 21:06 ` Dave Chinner 0 siblings, 1 reply; 4+ messages in thread From: Eric Sandeen @ 2016-01-30 3:26 UTC (permalink / raw) To: xfs Commit eef334e added an ASSERT that the inode was locked in some way in xfs_bmapi_read(), but on realtime paths through xfs_rtbuf_get() this isn't the case; fix that. Reported-by: Ross Zwisler <ross.zwisler@linux.intel.com> Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- I think we need the data_map_shared gyrations here, but not certain... diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c index 9b59ffa..e6da0b2 100644 --- a/fs/xfs/libxfs/xfs_rtbitmap.c +++ b/fs/xfs/libxfs/xfs_rtbitmap.c @@ -57,11 +57,14 @@ xfs_rtbuf_get( xfs_inode_t *ip; /* bitmap or summary inode */ xfs_bmbt_irec_t map; int nmap = 1; + int lock_mode; int error; /* error value */ ip = issum ? mp->m_rsumip : mp->m_rbmip; + lock_mode = xfs_ilock_data_map_shared(ip); error = xfs_bmapi_read(ip, block, 1, &map, &nmap, XFS_DATA_FORK); + xfs_iunlock(ip, lock_mode); if (error) return error; _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs: lock bitmap/summary inodes in xfs_rtbuf_get() 2016-01-30 3:26 [PATCH] xfs: lock bitmap/summary inodes in xfs_rtbuf_get() Eric Sandeen @ 2016-01-30 21:06 ` Dave Chinner 2016-01-30 21:53 ` Eric Sandeen 0 siblings, 1 reply; 4+ messages in thread From: Dave Chinner @ 2016-01-30 21:06 UTC (permalink / raw) To: Eric Sandeen; +Cc: xfs On Fri, Jan 29, 2016 at 09:26:28PM -0600, Eric Sandeen wrote: > Commit eef334e added an ASSERT that the inode was locked in > some way in xfs_bmapi_read(), but on realtime paths through > xfs_rtbuf_get() this isn't the case; fix that. > > Reported-by: Ross Zwisler <ross.zwisler@linux.intel.com> > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > > I think we need the data_map_shared gyrations here, but not certain... > > diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c > index 9b59ffa..e6da0b2 100644 > --- a/fs/xfs/libxfs/xfs_rtbitmap.c > +++ b/fs/xfs/libxfs/xfs_rtbitmap.c > @@ -57,11 +57,14 @@ xfs_rtbuf_get( > xfs_inode_t *ip; /* bitmap or summary inode */ > xfs_bmbt_irec_t map; > int nmap = 1; > + int lock_mode; > int error; /* error value */ > > ip = issum ? mp->m_rsumip : mp->m_rbmip; > > + lock_mode = xfs_ilock_data_map_shared(ip); > error = xfs_bmapi_read(ip, block, 1, &map, &nmap, XFS_DATA_FORK); > + xfs_iunlock(ip, lock_mode); I've looked into this recently and didn't think up a simple answer to the problem. I didn't spend much time on it because it's nowhere near the top of my priority list because it only affects debug kernels as the summary inode is effectively protected by the bitmap inode exclusion during allocation. That said, the above change is not safe because xfs_rtbuf_get() can be called with the bitmap inode lock already held. e.g: xfs_bmap_rtalloc xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL); << locks bitmap xfs_rtallocate_extent xfs_rtallocate_extent_exact xfs_rtcheck_range xfs_rtbuf_get(issum = 0) xfs_ilock_data_map_shared(mp->m_rbmip) <self deadlock> The issue here is that the summary inode is not locked early on in the transaction, so modifications are done with it unlocked. xfs_bmap_rtalloc xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL); << locks bitmap xfs_rtallocate_extent xfs_rtallocate_extent_size xfs_rtget_summary xfs_rtmodify_summary_int xfs_rtbuf_get xfs_bmapi_read(summary inode) << unlocked summary The only path through which the summary inode is locked for modification is the growfs path (xfs_rtcopy_summary()), so all the other paths that modify/access the summary inode also need to be locked at a higher level before calling into the summary functions. Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs: lock bitmap/summary inodes in xfs_rtbuf_get() 2016-01-30 21:06 ` Dave Chinner @ 2016-01-30 21:53 ` Eric Sandeen 2016-02-01 22:05 ` Dave Chinner 0 siblings, 1 reply; 4+ messages in thread From: Eric Sandeen @ 2016-01-30 21:53 UTC (permalink / raw) To: Dave Chinner; +Cc: Eric Sandeen, xfs OK, thanks Dave. It seemed like it was probably too simple… > On Jan 30, 2016, at 3:06 PM, Dave Chinner <david@fromorbit.com> wrote: > >> On Fri, Jan 29, 2016 at 09:26:28PM -0600, Eric Sandeen wrote: >> Commit eef334e added an ASSERT that the inode was locked in >> some way in xfs_bmapi_read(), but on realtime paths through >> xfs_rtbuf_get() this isn't the case; fix that. >> >> Reported-by: Ross Zwisler <ross.zwisler@linux.intel.com> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >> --- >> >> I think we need the data_map_shared gyrations here, but not certain... >> >> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c >> index 9b59ffa..e6da0b2 100644 >> --- a/fs/xfs/libxfs/xfs_rtbitmap.c >> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c >> @@ -57,11 +57,14 @@ xfs_rtbuf_get( >> xfs_inode_t *ip; /* bitmap or summary inode */ >> xfs_bmbt_irec_t map; >> int nmap = 1; >> + int lock_mode; >> int error; /* error value */ >> >> ip = issum ? mp->m_rsumip : mp->m_rbmip; >> >> + lock_mode = xfs_ilock_data_map_shared(ip); >> error = xfs_bmapi_read(ip, block, 1, &map, &nmap, XFS_DATA_FORK); >> + xfs_iunlock(ip, lock_mode); > > I've looked into this recently and didn't think up a simple answer > to the problem. I didn't spend much time on it because it's nowhere > near the top of my priority list because it only affects debug > kernels as the summary inode is effectively protected by the bitmap > inode exclusion during allocation. > > That said, the above change is not safe because xfs_rtbuf_get() can > be called with the bitmap inode lock already held. e.g: > > xfs_bmap_rtalloc > xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL); << locks bitmap > xfs_rtallocate_extent > xfs_rtallocate_extent_exact > xfs_rtcheck_range > xfs_rtbuf_get(issum = 0) > xfs_ilock_data_map_shared(mp->m_rbmip) > <self deadlock> > > The issue here is that the summary inode is not locked early on in > the transaction, so modifications are done with it unlocked. > > xfs_bmap_rtalloc > xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL); << locks bitmap > xfs_rtallocate_extent > xfs_rtallocate_extent_size > xfs_rtget_summary > xfs_rtmodify_summary_int > xfs_rtbuf_get > xfs_bmapi_read(summary inode) << unlocked summary > > The only path through which the summary inode is locked for > modification is the growfs path (xfs_rtcopy_summary()), so all the > other paths that modify/access the summary inode also need to be > locked at a higher level before calling into the summary functions. > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com > > _______________________________________________ > xfs mailing list > xfs@oss.sgi.com > http://oss.sgi.com/mailman/listinfo/xfs > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs: lock bitmap/summary inodes in xfs_rtbuf_get() 2016-01-30 21:53 ` Eric Sandeen @ 2016-02-01 22:05 ` Dave Chinner 0 siblings, 0 replies; 4+ messages in thread From: Dave Chinner @ 2016-02-01 22:05 UTC (permalink / raw) To: Eric Sandeen; +Cc: Eric Sandeen, xfs On Sat, Jan 30, 2016 at 03:53:48PM -0600, Eric Sandeen wrote: > OK, thanks Dave. It seemed like it was probably too simple… Yeah, and the simple fix (lock in xfs_bmap_rtalloc()) uncovers further issues w.r.t. logging - the buffers are not stamped with a type, so log recovery will not categorise them correctly. And, I suspect, log recovery might have problems with the bitmap/smmary buffers as they don't have magic numbers in them. More work to be done here... Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-02-01 22:05 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-01-30 3:26 [PATCH] xfs: lock bitmap/summary inodes in xfs_rtbuf_get() Eric Sandeen 2016-01-30 21:06 ` Dave Chinner 2016-01-30 21:53 ` Eric Sandeen 2016-02-01 22:05 ` Dave Chinner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox