On Mon, Apr 30, 2018 at 10:46:37AM -0500, Eric Sandeen wrote:
> The GET ioctl is trivial, just return the current label.
>
> The SET ioctl is more involved:
> It transactionally modifies the superblock to write a new filesystem
> label to the primary super.
>
> It then also checkpoints the log to disk so that the change lands in
> block 0, invalidates any page cache that userspace might have previously
> read, and updates all secondary superblocks as userspace relable does.
>
> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> ---
Just to close the loop on what Eric and I just talked about on
IRC....
> +xfs_ioc_setlabel(
> + struct file *filp,
> + struct xfs_mount *mp,
> + char __user *newlabel)
> +{
> + struct address_space *mapping;
> + struct xfs_sb *sbp = &mp->m_sb;
> + char label[FSLABEL_MAX];
> + size_t len;
> + int error;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + if (copy_from_user(label, newlabel, FSLABEL_MAX))
> + return -EFAULT;
> + /*
> + * The generic ioctl allows up to FSLABEL_MAX chars, but xfs is much
> + * smaller, at 12 bytes.
> + * NB: The on disk label doesn't need to be null terminated.
> + */
> + len = strnlen(label, FSLABEL_MAX);
> + if (len > sizeof(sbp->sb_fname)) {
> + xfs_warn(mp, "label %s is too long, max %zu chars",
> + label, sizeof(sbp->sb_fname));
> + return -EINVAL;
> + }
> +
> + error = mnt_want_write_file(filp);
> + if (error)
> + return error;
> +
> + spin_lock(&mp->m_sb_lock);
> + memset(sbp->sb_fname, 0, sizeof(sbp->sb_fname));
> + strncpy(sbp->sb_fname, label, sizeof(sbp->sb_fname));
> + spin_unlock(&mp->m_sb_lock);
> +
> + error = xfs_sync_sb(mp, true);
> + if (error)
> + goto out;
This is all good up to here. We want a synchronous transaction to
force the change to the log and *unpin the superblock buffer* at the
same time.
However, we want to also write the superblock buffer, and so we
really should have a function that does this directly: i.e.
/* Force superblock label changes to disk */
error = xfs_sync_sb_buf(mp);
if (error)
goto out;
which does:
xfs_sync_sb_buf()
{
/* same transaction preamble and logging as xfs_sync_sb() */
.....
xfs_trans_bhold(tp, bp);
xfs_trans_set_sync(tp);
error = xfs_trans_commit(tp);
if (error)
goto out;
/*
* write out the sb buffer to get the changes to disk
*/
error = xfs_bwrite(bp);
xfs_buf_relse(bp);
return error;
}
> + /* checkpoint the log to update primary super in fs itself */
> + error = xfs_log_checkpoint(mp);
> + if (error)
> + goto out;
> +
> + /* 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;
invalidate_bdev()? We don't use this block device
mapping, so just invalidating it completely is probably appropriate.
FWIW, ISTR that userspace points at a different address space
mapping and we can't invalidate that directly ourselves? Worth
checking...
> + /* 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);
> + }
Why not just block here?
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
--
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
