Re: [PATCH v4 3/6] xfs: implement write-stream management support
Christoph Hellwig <[email protected]>
| Newsgroups | org.kernel.vger.linux-xfs,org.kernel.vger.linux-block,org.kernel.vger.linux-fsdevel |
|---|---|
| Message-ID | <[email protected]> |
> +int
> +xfs_inode_max_write_streams(
> + struct xfs_inode *ip)
> +{
> + struct block_device *bdev;
> + bool is_filestream, is_realtime;
> +
> + xfs_ilock(ip, XFS_ILOCK_SHARED);
> + is_filestream = xfs_inode_is_filestream(ip);
> + is_realtime = XFS_IS_REALTIME_INODE(ip);
> + bdev = xfs_inode_buftarg(ip)->bt_bdev;
> + xfs_iunlock(ip, XFS_ILOCK_SHARED);
> +
> + if (!bdev || is_filestream || is_realtime)
> + return 0;
All this information is stale as soon as soon as the lock is
dropped. Also a NULL bdev can only happen for the in-memory
buftarg used by repair, so no need to check this.
> +uint16_t
> +xfs_inode_get_write_stream(
> + struct xfs_inode *ip)
> +{
> + uint16_t stream_id;
> +
> + xfs_ilock(ip, XFS_ILOCK_SHARED);
> + stream_id = ip->i_write_stream;
> + xfs_iunlock(ip, XFS_ILOCK_SHARED);
Same here. READ_ONCE/WRITE_ONCE might be a better idea to simply
avoid the lock at read time.
> +{
> + struct xfs_write_stream *ws = file->private_data;
> + struct xfs_mount *mp = ws->mp;
> +
> + spin_lock(&mp->m_streams_lock);
> + clear_bit(ws->stream_id - 1, mp->m_streams_in_use);
Given that all accesses to the m_streams_in_use bitmap use a lock,
there is no need for the atomic bitops. We could use the __-versions
or just code the logic. Or not bother because none of this is
performance critical :)
> + ws = fd_file(f)->private_data;
> + if (ws->mp != ip->i_mount)
> + return -EINVAL;
> +
> + xfs_ilock(ip, XFS_ILOCK_EXCL);
> +
> + if (XFS_IS_REALTIME_INODE(ip) || xfs_inode_is_filestream(ip) ||
> + VFS_I(ip)->i_write_hint != WRITE_LIFE_NOT_SET) {
If we want to enforce a hint vs stream exlusion we'd also need to
do this when setting the hints. Another approach might be to allow
them to coexist, and set a lifetime hint on each stream, which the
inode must match. Although I'm not sure this would be all that
useful.