Messages in this thread | | | Date | Wed, 14 Feb 2024 12:26:27 +0000 | Subject | Re: [PATCH 4/6] fs: xfs: Support atomic write for statx | From | John Garry <> |
| |
On 13/02/2024 17:37, Darrick J. Wong wrote: >> We use this in the iomap and statx code >> >>>> + struct xfs_inode *ip, >>>> + unsigned int *unit_min, >>>> + unsigned int *unit_max) >>> Weird indenting here. >> hmmm... I thought that this was the XFS style >> >> Can you show how it should look? > The parameter declarations should line up with the local variables: > > void > xfs_get_atomic_write_attr( > struct xfs_inode *ip, > unsigned int *unit_min, > unsigned int *unit_max) > { > struct xfs_buftarg *target = xfs_inode_buftarg(ip); > struct block_device *bdev = target->bt_bdev; > struct request_queue *q = bdev->bd_queue; > struct xfs_mount *mp = ip->i_mount; > unsigned int awu_min, awu_max, align; > xfs_extlen_t extsz = xfs_get_extsz(ip); > >>>> +{ >>>> + xfs_extlen_t extsz = xfs_get_extsz(ip); >>>> + struct xfs_buftarg *target = xfs_inode_buftarg(ip); >>>> + struct block_device *bdev = target->bt_bdev; >>>> + unsigned int awu_min, awu_max, align; >>>> + struct request_queue *q = bdev->bd_queue; >>>> + struct xfs_mount *mp = ip->i_mount; >>>> + >>>> + /* >>>> + * Convert to multiples of the BLOCKSIZE (as we support a minimum >>>> + * atomic write unit of BLOCKSIZE). >>>> + */ >>>> + awu_min = queue_atomic_write_unit_min_bytes(q); >>>> + awu_max = queue_atomic_write_unit_max_bytes(q); >>>> + >>>> + awu_min &= ~mp->m_blockmask; >>> Why do you round/down/ the awu_min value here? >> This is just to ensure that we returning *unit_min >= BLOCKSIZE >> >> For example, if awu_min, max 1K, 64K from the bdev, we now have 0 and 64K. >> And below this gives us awu_min, max of 4k, 64k. >> >> Maybe there is a more logical way of doing this. > awu_min = roundup(queue_atomic_write_unit_min_bytes(q), > mp->m_sb.sb_blocksize); > > ?
Yeah, I think that all this can be simplified to be made more obvious.
> >>>> + awu_max &= ~mp->m_blockmask; >>> Actually -- since the atomic write units have to be powers of 2, why is >>> rounding needed here at all? >> Sure, but the bdev can report a awu_min < BLOCKSIZE >> >>>> + >>>> + align = XFS_FSB_TO_B(mp, extsz); >>>> + >>>> + if (!awu_max || !xfs_inode_atomicwrites(ip) || !align || >>>> + !is_power_of_2(align)) { >>> ...and if you take my suggestion to make a common helper to validate the >>> atomic write unit parameters, this can collapse into: >>> >>> alloc_unit_bytes = xfs_inode_alloc_unitsize(ip); >>> if (!xfs_inode_has_atomicwrites(ip) || >>> !bdev_validate_atomic_write(bdev, alloc_unit_bytes)) > /* not supported, return zeroes */ >>> *unit_min = 0; >>> *unit_max = 0; >>> return; >>> } >>> >>> *unit_min = max(alloc_unit_bytes, awu_min); >>> *unit_max = min(alloc_unit_bytes, awu_max); >> Again, we need to ensure that *unit_min >= BLOCKSIZE > The file allocation unit and hence the return value of > xfs_inode_alloc_unitsize is always a multiple of sb_blocksize.
Right, but this value is coming from HW and we are just ensuring that the awu_min which we report is >= BLOCKSIZE. xfs_inode_alloc_unitsize() return value will really guide unit_max.
Anyway, again I can make this all more obvious.
Thanks, John
| |