On Mon, Oct 12, 2020 at 11:02:51PM +0200, Pavel Reichl wrote:
>
> > ...
> >> @@ -384,16 +385,17 @@ xfs_isilocked(
> >> struct xfs_inode *ip,
> >> uint lock_flags)
> >> {
> >> - if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
> >> - if (!(lock_flags & XFS_ILOCK_SHARED))
> >> - return !!ip->i_lock.mr_writer;
> >> - return rwsem_is_locked(&ip->i_lock.mr_lock);
> >> + if (lock_flags & (XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)) {
> >> + ASSERT(!(lock_flags & ~(XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)));
> >> + return __xfs_rwsem_islocked(&ip->i_lock,
> >> + (lock_flags >> XFS_ILOCK_FLAG_SHIFT));
> >> }
> >>
> >> - if (lock_flags & (XFS_MMAPLOCK_EXCL|XFS_MMAPLOCK_SHARED)) {
> >> - if (!(lock_flags & XFS_MMAPLOCK_SHARED))
> >> - return !!ip->i_mmaplock.mr_writer;
> >> - return rwsem_is_locked(&ip->i_mmaplock.mr_lock);
> >> + if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)) {
> >> + ASSERT(!(lock_flags &
> >> + ~(XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)));
> >> + return __xfs_rwsem_islocked(&ip->i_mmaplock,
> >> + (lock_flags >> XFS_MMAPLOCK_FLAG_SHIFT));
> >> }
> >>
> >> if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) {
> >
> > Can we add a similar assert for this case as we have for the others?
> > Otherwise the rest looks fairly straightforward to me.
> >
>
> Sure we can! But do we want to?
>
> I think that these asserts are supposed to make sure that only flags
> for one of the inode's locks are used eg. ILOCK, MMAPLOCK or IOLOCK
> but no combination! So if we reach this 3rd condition we already know
> that the flags for ILOCK and MMAPLOCK were not set. However if there's
> possibility for more locks to be added in the future or just for the
> 'code symmetry' purposes - I have no problem to update the code.
It's generally a good idea not to leave logic bombs of the sort where
where the debugging code can bitrot into incorrectness if someone
unwittingly adds another level of locking later.
(That said, I really hope we don't; I already consider it a little
strange to have separate io and mmap locks...)
--D
