On Tue, Jul 24, 2018 at 03:35:23PM -0700, Darrick J. Wong wrote: > > mutex_unlock contains a barrier, and the sequence is a single register > > read. There is nothing holding a lock here would help us with. > > Which mutex_unlock is that? Actually an up_write (on i_lock), but the result is the same. > I took a second look at the i_cowfp access > and realized that we don't take ILOCK_SHARED until after the comparison. > Writeback doesn't take any of the other inode locks AFAIK... so either > there's some locking subtlety here that ought to be explained in a > comment, or is this a bug waiting to happen? What would you want the lock to protect? As said above the if_seq field itself doesn't need a lock, it can be read atomic because it is a register or less. And for the actual imap we don't have a lock anywhere in the writeback code - as soon as we did the lookup or allocation we drop the i_lock and keep using it. That is generally fine because we only ever remove extents from it after previously waiting for writeback and using the iolock to prevent new writeback from being started. The only new bit with reflink support is that a valid data fork block mapping might now be shadowed by a new reflink mapping, and that is exactly what the sequence count protects against (instead of doing a lookup in the cowfp and then instanctly dropping the lock before) -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
