OK, thanks Dave. It seemed like it was probably too simple… > On Jan 30, 2016, at 3:06 PM, Dave Chinner <david@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> >> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx> >> --- >> >> 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@xxxxxxxxxxxxx > > _______________________________________________ > xfs mailing list > xfs@xxxxxxxxxxx > http://oss.sgi.com/mailman/listinfo/xfs > _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
