On 5/1/18 9:42 AM, Brian Foster wrote:
> On Tue, May 01, 2018 at 09:27:30AM -0500, Eric Sandeen wrote:
>>>> + /* invalidate any cached bdev page for userspace visibility */
>>>> + mapping = mp->m_ddev_targp->bt_bdev->bd_inode->i_mapping;
>>>> + error = invalidate_inode_pages2_range(mapping, 0, 0);
>>>> + if (error)
>>>> + goto out;
>>>
>>> A more detailed comment on why this is necessary would be helpful. :)
>>
>> Why we need to invalidate cached pages on the block device? I thought that
>> was clear from the comment, but I can add more words.
>>
>
> I'm not really sure what "userspace visibility" means. Wouldn't
> userspace just call the get label ioctl?
It could, but we also have blkid which reads the block device directly
today, even when mounted. They need to co-exist, and blkid needs to see
the label change /on the block device/ after it's modified while mounted.
>> /*
>> * The block device may have cached pages for the block device from previous
>> * reads in userspace, and we want i.e. blkid to see the new info, so invalidate
>> * any cached pages now.
>> */
>>
>> Like that?
>>
>
> I suppose. Does something actually break without this? Why do we care
> about bdev inode cache invalidation here and not with any other
> superblock modifications?
Because there's no other situation or utility that I'm aware of which reads
a mounted block device directly, and is gathering data which may now change
while mounted.
IOWSs, blkid reads things like label, uuid, fs type, etc, but none of those
can (could) change while mounted; now label /can/ change, and blkid must
see it.
i.e. we don't want:
# blkid -s LABEL /dev/sdc1
/dev/sdc1: LABEL="oldlabel"
# xfs_io -c "label newlabel" /mnt/sdc1
label = newlabel
# blkid -s LABEL /dev/sdc1
/dev/sdc1: LABEL="oldlabel"
- we want to see the /new/ label.
>>>> +
>>>> + /* update the backup superblocks like userspace does */
>>>> + if (mutex_trylock(&mp->m_growlock)) {
>>>> + error = xfs_update_secondary_supers(mp, mp->m_sb.sb_agcount, 0);
>>>> + mutex_unlock(&mp->m_growlock);
>>>> + }
>>>
>>> It looks like growfs returns an error if the lock not available. Did you
>>> intend to perform the primary update and then skip the secondary updates
>>> if the lock is not acquired?
>>>
>>> Perhaps we should just trylock earlier and hold the lock across the
>>> entire operation..? It's not like grow or relabel are frequent
>>> operations.
>>
>> yeah, I was on the fence about this. Updating the secondaries with the new label
>> is not strictly required; repair doesn't care at all, and online repair considers
>> it a "preen" action. TBH I was lazily unsure about growlock vs. m_sb_lock nesting
>> and didn't want to risk getting it wrong.
>>
>
> I have no strong preference either way, but ISTM we should decide
> whether to update secondary supers or not.
Ok. I'll wait for the growfs lock to ensure we do the secondary updates, I guess.
Doing everything else and then returning -EBUSY for the secondaries would be
confusing IMHO.
-Eric
--
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
