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.
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.