| Message ID | 20240817094800.776408-8-john.g.garry@oracle.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | block atomic writes for xfs | expand |
On Sat, Aug 17, 2024 at 09:48:00AM +0000, John Garry wrote: > For when an inode is enabled for atomic writes, set FMODE_CAN_ATOMIC_WRITE > flag. Only direct IO is currently supported, so check for that also. > > We rely on the block layer to reject atomic writes which exceed the bdev > request_queue limits, so don't bother checking any such thing here. > > Signed-off-by: John Garry <john.g.garry@oracle.com> > --- > fs/xfs/xfs_file.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > index 9b6530a4eb4a..3489d478809e 100644 > --- a/fs/xfs/xfs_file.c > +++ b/fs/xfs/xfs_file.c > @@ -1149,6 +1149,18 @@ xfs_file_remap_range( > return remapped > 0 ? remapped : ret; > } > > +static bool xfs_file_open_can_atomicwrite( > + struct inode *inode, > + struct file *file) > +{ > + struct xfs_inode *ip = XFS_I(inode); > + > + if (!(file->f_flags & O_DIRECT)) > + return false; > + > + return xfs_inode_has_atomicwrites(ip); ...and here too. I do like the shift to having an incore flag that controls whether you get untorn write support or not. --D > +} > + > STATIC int > xfs_file_open( > struct inode *inode, > @@ -1157,6 +1169,8 @@ xfs_file_open( > if (xfs_is_shutdown(XFS_M(inode->i_sb))) > return -EIO; > file->f_mode |= FMODE_NOWAIT | FMODE_CAN_ODIRECT; > + if (xfs_file_open_can_atomicwrite(inode, file)) > + file->f_mode |= FMODE_CAN_ATOMIC_WRITE; > return generic_file_open(inode, file); > } > > -- > 2.31.1 > >
On 21/08/2024 18:11, Darrick J. Wong wrote: > On Sat, Aug 17, 2024 at 09:48:00AM +0000, John Garry wrote: >> For when an inode is enabled for atomic writes, set FMODE_CAN_ATOMIC_WRITE >> flag. Only direct IO is currently supported, so check for that also. >> >> We rely on the block layer to reject atomic writes which exceed the bdev >> request_queue limits, so don't bother checking any such thing here. >> >> Signed-off-by: John Garry <john.g.garry@oracle.com> >> --- >> fs/xfs/xfs_file.c | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c >> index 9b6530a4eb4a..3489d478809e 100644 >> --- a/fs/xfs/xfs_file.c >> +++ b/fs/xfs/xfs_file.c >> @@ -1149,6 +1149,18 @@ xfs_file_remap_range( >> return remapped > 0 ? remapped : ret; >> } >> >> +static bool xfs_file_open_can_atomicwrite( >> + struct inode *inode, >> + struct file *file) >> +{ >> + struct xfs_inode *ip = XFS_I(inode); >> + >> + if (!(file->f_flags & O_DIRECT)) >> + return false; >> + >> + return xfs_inode_has_atomicwrites(ip); > > ...and here too. I do like the shift to having an incore flag that > controls whether you get untorn write support or not. Do you mean that add a new member to xfs_inode to record this? If yes, it sounds ok, but we need to maintain consistency (of that member) whenever anything which can affect it changes, which is always a bit painful. John
On Thu, Aug 22, 2024 at 07:04:02PM +0100, John Garry wrote: > On 21/08/2024 18:11, Darrick J. Wong wrote: > > On Sat, Aug 17, 2024 at 09:48:00AM +0000, John Garry wrote: > > > For when an inode is enabled for atomic writes, set FMODE_CAN_ATOMIC_WRITE > > > flag. Only direct IO is currently supported, so check for that also. > > > > > > We rely on the block layer to reject atomic writes which exceed the bdev > > > request_queue limits, so don't bother checking any such thing here. > > > > > > Signed-off-by: John Garry <john.g.garry@oracle.com> > > > --- > > > fs/xfs/xfs_file.c | 14 ++++++++++++++ > > > 1 file changed, 14 insertions(+) > > > > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > > > index 9b6530a4eb4a..3489d478809e 100644 > > > --- a/fs/xfs/xfs_file.c > > > +++ b/fs/xfs/xfs_file.c > > > @@ -1149,6 +1149,18 @@ xfs_file_remap_range( > > > return remapped > 0 ? remapped : ret; > > > } > > > +static bool xfs_file_open_can_atomicwrite( > > > + struct inode *inode, > > > + struct file *file) > > > +{ > > > + struct xfs_inode *ip = XFS_I(inode); > > > + > > > + if (!(file->f_flags & O_DIRECT)) > > > + return false; > > > + > > > + return xfs_inode_has_atomicwrites(ip); > > > > ...and here too. I do like the shift to having an incore flag that > > controls whether you get untorn write support or not. > > Do you mean that add a new member to xfs_inode to record this? If yes, it > sounds ok, but we need to maintain consistency (of that member) whenever > anything which can affect it changes, which is always a bit painful. I actually meant something more like: static bool xfs_file_open_can_atomicwrite( struct file *file, struct inode *inode) { struct xfs_inode *ip = XFS_I(inode); struct xfs_mount *mp = ip->i_mount; struct xfs_buftarg *target = xfs_inode_buftarg(ip); if (!(file->f_flags & O_DIRECT)) return false; if (!xfs_inode_has_atomicwrites(ip)) return false; if (mp->m_dalign && (mp->m_dalign % ip->i_extsize)) return false; if (mp->m_swidth && (mp->m_swidth % ip->i_extsize)) return false; if (mp->m_sb.sb_blocksize < target->bt_bdev_awu_min) return false; if (xfs_inode_alloc_unitsize(ip) > target->bt_bdev_awu_max) return false; return true; } --D > John >
On 22/08/2024 21:44, Darrick J. Wong wrote: >> Do you mean that add a new member to xfs_inode to record this? If yes, it >> sounds ok, but we need to maintain consistency (of that member) whenever >> anything which can affect it changes, which is always a bit painful. > I actually meant something more like: > > static bool > xfs_file_open_can_atomicwrite( > struct file *file, > struct inode *inode) > { > struct xfs_inode *ip = XFS_I(inode); > struct xfs_mount *mp = ip->i_mount; > struct xfs_buftarg *target = xfs_inode_buftarg(ip); > > if (!(file->f_flags & O_DIRECT)) > return false; > if (!xfs_inode_has_atomicwrites(ip)) > return false; > if (mp->m_dalign && (mp->m_dalign % ip->i_extsize)) > return false; > if (mp->m_swidth && (mp->m_swidth % ip->i_extsize)) > return false; > if (mp->m_sb.sb_blocksize < target->bt_bdev_awu_min) > return false; > if (xfs_inode_alloc_unitsize(ip) > target->bt_bdev_awu_max) > return false; > return true; > } ok, but we should probably factor out some duplicated code with helpers, like: bool xfs_validate_atomicwrites_extsize(struct xfs_mount *mp, uint32_t extsize) { if (!is_power_of_2(extsize)) return false; /* Required to guarantee data block alignment */ if (mp->m_sb.sb_agblocks % extsize) return false; /* Requires stripe unit+width be a multiple of extsize */ if (mp->m_dalign && (mp->m_dalign % extsize)) return false; if (mp->m_swidth && (mp->m_swidth % extsize)) return false; return true; } bool xfs_inode_has_atomicwrites(struct xfs_inode *ip) { struct xfs_mount *mp = ip->i_mount; struct xfs_buftarg *target = xfs_inode_buftarg(ip); if (!(ip->i_diflags2 & XFS_DIFLAG2_ATOMICWRITES)) return false; if (!xfs_validate_atomicwrites_extsize(mp, ip->i_extsize)) return false; if (mp->m_sb.sb_blocksize < target->bt_bdev_awu_min) return false; if (xfs_inode_alloc_unitsize(ip) > target->bt_bdev_awu_max) return false; return true; } static bool xfs_file_open_can_atomicwrite( struct inode *inode, struct file *file) { struct xfs_inode *ip = XFS_I(inode); if (!(file->f_flags & O_DIRECT)) return false; return xfs_inode_has_atomicwrites(ip); } Those helpers can be re-used in xfs_inode_validate_atomicwrites() and xfs_ioctl_setattr_atomicwrites(). John
On Fri, Aug 23, 2024 at 11:41:07AM +0100, John Garry wrote: > On 22/08/2024 21:44, Darrick J. Wong wrote: > > > Do you mean that add a new member to xfs_inode to record this? If yes, it > > > sounds ok, but we need to maintain consistency (of that member) whenever > > > anything which can affect it changes, which is always a bit painful. > > I actually meant something more like: > > > > static bool > > xfs_file_open_can_atomicwrite( > > struct file *file, > > struct inode *inode) > > { > > struct xfs_inode *ip = XFS_I(inode); > > struct xfs_mount *mp = ip->i_mount; > > struct xfs_buftarg *target = xfs_inode_buftarg(ip); > > > > if (!(file->f_flags & O_DIRECT)) > > return false; > > if (!xfs_inode_has_atomicwrites(ip)) > > return false; > > if (mp->m_dalign && (mp->m_dalign % ip->i_extsize)) > > return false; > > if (mp->m_swidth && (mp->m_swidth % ip->i_extsize)) > > return false; > > if (mp->m_sb.sb_blocksize < target->bt_bdev_awu_min) > > return false; > > if (xfs_inode_alloc_unitsize(ip) > target->bt_bdev_awu_max) > > return false; > > return true; > > } > > ok, but we should probably factor out some duplicated code with helpers, > like: > > bool xfs_validate_atomicwrites_extsize(struct xfs_mount *mp, uint32_t > extsize) xfs_agblock_t extsize, but other than that this looks right to me. > { > if (!is_power_of_2(extsize)) > return false; > > /* Required to guarantee data block alignment */ > if (mp->m_sb.sb_agblocks % extsize) > return false; > > /* Requires stripe unit+width be a multiple of extsize */ > if (mp->m_dalign && (mp->m_dalign % extsize)) > return false; > > if (mp->m_swidth && (mp->m_swidth % extsize)) > return false; > > return true; > } > > > bool xfs_inode_has_atomicwrites(struct xfs_inode *ip) > { > struct xfs_mount *mp = ip->i_mount; > struct xfs_buftarg *target = xfs_inode_buftarg(ip); > > if (!(ip->i_diflags2 & XFS_DIFLAG2_ATOMICWRITES)) > return false; > if (!xfs_validate_atomicwrites_extsize(mp, ip->i_extsize)) > return false; > if (mp->m_sb.sb_blocksize < target->bt_bdev_awu_min) > return false; > if (xfs_inode_alloc_unitsize(ip) > target->bt_bdev_awu_max) > return false; > return true; > } > > > static bool xfs_file_open_can_atomicwrite( > struct inode *inode, > struct file *file) > { > struct xfs_inode *ip = XFS_I(inode); > > if (!(file->f_flags & O_DIRECT)) > return false; > return xfs_inode_has_atomicwrites(ip); > } > > Those helpers can be re-used in xfs_inode_validate_atomicwrites() and > xfs_ioctl_setattr_atomicwrites(). Looks good to me. --D > > John > >
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index 9b6530a4eb4a..3489d478809e 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -1149,6 +1149,18 @@ xfs_file_remap_range( return remapped > 0 ? remapped : ret; } +static bool xfs_file_open_can_atomicwrite( + struct inode *inode, + struct file *file) +{ + struct xfs_inode *ip = XFS_I(inode); + + if (!(file->f_flags & O_DIRECT)) + return false; + + return xfs_inode_has_atomicwrites(ip); +} + STATIC int xfs_file_open( struct inode *inode, @@ -1157,6 +1169,8 @@ xfs_file_open( if (xfs_is_shutdown(XFS_M(inode->i_sb))) return -EIO; file->f_mode |= FMODE_NOWAIT | FMODE_CAN_ODIRECT; + if (xfs_file_open_can_atomicwrite(inode, file)) + file->f_mode |= FMODE_CAN_ATOMIC_WRITE; return generic_file_open(inode, file); }
For when an inode is enabled for atomic writes, set FMODE_CAN_ATOMIC_WRITE flag. Only direct IO is currently supported, so check for that also. We rely on the block layer to reject atomic writes which exceed the bdev request_queue limits, so don't bother checking any such thing here. Signed-off-by: John Garry <john.g.garry@oracle.com> --- fs/xfs/xfs_file.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)