| Message ID | 20240124142645.9334-7-john.g.garry@oracle.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | block atomic writes for XFS | expand |
On Wed, Jan 24, 2024 at 02:26:45PM +0000, John Garry wrote: > For when an inode is enabled for atomic writes, set FMODE_CAN_ATOMIC_WRITE > flag. > > Signed-off-by: John Garry <john.g.garry@oracle.com> > --- > fs/xfs/xfs_file.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > index e33e5e13b95f..1375d0089806 100644 > --- a/fs/xfs/xfs_file.c > +++ b/fs/xfs/xfs_file.c > @@ -1232,6 +1232,8 @@ xfs_file_open( > return -EIO; > file->f_mode |= FMODE_NOWAIT | FMODE_BUF_RASYNC | FMODE_BUF_WASYNC | > FMODE_DIO_PARALLEL_WRITE | FMODE_CAN_ODIRECT; > + if (xfs_inode_atomicwrites(XFS_I(inode))) Shouldn't we check that the device supports AWU at all before turning on the FMODE flag? --D > + file->f_mode |= FMODE_CAN_ATOMIC_WRITE; > return generic_file_open(inode, file); > } > > -- > 2.31.1 > >
On 02/02/2024 18:06, Darrick J. Wong wrote: > On Wed, Jan 24, 2024 at 02:26:45PM +0000, John Garry wrote: >> For when an inode is enabled for atomic writes, set FMODE_CAN_ATOMIC_WRITE >> flag. >> >> Signed-off-by: John Garry <john.g.garry@oracle.com> >> --- >> fs/xfs/xfs_file.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c >> index e33e5e13b95f..1375d0089806 100644 >> --- a/fs/xfs/xfs_file.c >> +++ b/fs/xfs/xfs_file.c >> @@ -1232,6 +1232,8 @@ xfs_file_open( >> return -EIO; >> file->f_mode |= FMODE_NOWAIT | FMODE_BUF_RASYNC | FMODE_BUF_WASYNC | >> FMODE_DIO_PARALLEL_WRITE | FMODE_CAN_ODIRECT; >> + if (xfs_inode_atomicwrites(XFS_I(inode))) Note to self: This should also check if O_DIRECT is set > > Shouldn't we check that the device supports AWU at all before turning on > the FMODE flag? Can we easily get this sort of bdev info here? Currently if we do try to issue an atomic write and AWU for the bdev is zero, then XFS iomap code will reject it. Thanks, John > > --D > >> + file->f_mode |= FMODE_CAN_ATOMIC_WRITE; >> return generic_file_open(inode, file); >> } >> >> -- >> 2.31.1 >> >>
On Mon, Feb 05, 2024 at 10:26:43AM +0000, John Garry wrote: > On 02/02/2024 18:06, Darrick J. Wong wrote: > > On Wed, Jan 24, 2024 at 02:26:45PM +0000, John Garry wrote: > > > For when an inode is enabled for atomic writes, set FMODE_CAN_ATOMIC_WRITE > > > flag. > > > > > > Signed-off-by: John Garry <john.g.garry@oracle.com> > > > --- > > > fs/xfs/xfs_file.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > > > index e33e5e13b95f..1375d0089806 100644 > > > --- a/fs/xfs/xfs_file.c > > > +++ b/fs/xfs/xfs_file.c > > > @@ -1232,6 +1232,8 @@ xfs_file_open( > > > return -EIO; > > > file->f_mode |= FMODE_NOWAIT | FMODE_BUF_RASYNC | FMODE_BUF_WASYNC | > > > FMODE_DIO_PARALLEL_WRITE | FMODE_CAN_ODIRECT; > > > + if (xfs_inode_atomicwrites(XFS_I(inode))) > > Note to self: This should also check if O_DIRECT is set > > > > > Shouldn't we check that the device supports AWU at all before turning on > > the FMODE flag? > > Can we easily get this sort of bdev info here? > > Currently if we do try to issue an atomic write and AWU for the bdev is > zero, then XFS iomap code will reject it. Hmm. Well, if we move towards pushing all the hardware checks out of xfs/iomap and into whatever goes on underneath submit_bio then I guess we don't need to check device support here at all. --D > Thanks, > John > > > > > --D > > > > > + file->f_mode |= FMODE_CAN_ATOMIC_WRITE; > > > return generic_file_open(inode, file); > > > } > > > -- > > > 2.31.1 > > > > > > > >
On 13/02/2024 17:59, Darrick J. Wong wrote: >>> Shouldn't we check that the device supports AWU at all before turning on >>> the FMODE flag? >> Can we easily get this sort of bdev info here? >> >> Currently if we do try to issue an atomic write and AWU for the bdev is >> zero, then XFS iomap code will reject it. > Hmm. Well, if we move towards pushing all the hardware checks out of > xfs/iomap and into whatever goes on underneath submit_bio then I guess > we don't need to check device support here at all. Yeah, I have been thinking about this. But I was still planning on putting a "bdev on atomic write" check here, as you mentioned. But is this a proper method to access the bdev for an xfs inode: STATIC bool xfs_file_can_atomic_write( struct xfs_inode *inode) { struct xfs_buftarg *target = xfs_inode_buftarg(inode); struct block_device *bdev = target->bt_bdev; if (!xfs_inode_atomicwrites(inode)) return false; return bdev_can_atomic_write(bdev); } I do notice the dax check in xfs_bmbt_to_iomap() when assigning iomap->bdev, which is creating some doubt? Thanks, John
On Wed, Feb 14, 2024 at 12:36:40PM +0000, John Garry wrote: > On 13/02/2024 17:59, Darrick J. Wong wrote: > > > > Shouldn't we check that the device supports AWU at all before turning on > > > > the FMODE flag? > > > Can we easily get this sort of bdev info here? > > > > > > Currently if we do try to issue an atomic write and AWU for the bdev is > > > zero, then XFS iomap code will reject it. > > Hmm. Well, if we move towards pushing all the hardware checks out of > > xfs/iomap and into whatever goes on underneath submit_bio then I guess > > we don't need to check device support here at all. > > Yeah, I have been thinking about this. But I was still planning on putting a > "bdev on atomic write" check here, as you mentioned. > > But is this a proper method to access the bdev for an xfs inode: > > STATIC bool > xfs_file_can_atomic_write( > struct xfs_inode *inode) > { > struct xfs_buftarg *target = xfs_inode_buftarg(inode); > struct block_device *bdev = target->bt_bdev; > > if (!xfs_inode_atomicwrites(inode)) > return false; > > return bdev_can_atomic_write(bdev); > } There's still a TOCTOU race problem if the bdev gets reconfigured between xfs_file_can_atomic_write and submit_bio. However, if you're only using this to advertise the capability via statx then I suppose that's fine -- userspace has to have some means of discovering the ability at all. Userspace is also inherently racy. > I do notice the dax check in xfs_bmbt_to_iomap() when assigning iomap->bdev, > which is creating some doubt? Do you mean this? if (mapping_flags & IOMAP_DAX) iomap->dax_dev = target->bt_daxdev; else iomap->bdev = target->bt_bdev; The dax path wants dax_dev set so that it can do the glorified memcpy operation, and it doesn't need (or want) a block device. --D > Thanks, > John >
On 21/02/2024 17:00, Darrick J. Wong wrote: >>> Hmm. Well, if we move towards pushing all the hardware checks out of >>> xfs/iomap and into whatever goes on underneath submit_bio then I guess >>> we don't need to check device support here at all. >> Yeah, I have been thinking about this. But I was still planning on putting a >> "bdev on atomic write" check here, as you mentioned. >> >> But is this a proper method to access the bdev for an xfs inode: >> >> STATIC bool >> xfs_file_can_atomic_write( >> struct xfs_inode *inode) >> { >> struct xfs_buftarg *target = xfs_inode_buftarg(inode); >> struct block_device *bdev = target->bt_bdev; >> >> if (!xfs_inode_atomicwrites(inode)) >> return false; >> >> return bdev_can_atomic_write(bdev); >> } > There's still a TOCTOU race problem if the bdev gets reconfigured > between xfs_file_can_atomic_write and submit_bio. If that is the case then a check in the bio submit path is required to catch any such reconfigure problems - and we effectively have that in this series. I am looking at change some of these XFS bdev_can_atomic_write() checks, but would still have a check in the bio submit path. > > However, if you're only using this to advertise the capability via statx > then I suppose that's fine -- userspace has to have some means of > discovering the ability at all. Userspace is also inherently racy. > >> I do notice the dax check in xfs_bmbt_to_iomap() when assigning iomap->bdev, >> which is creating some doubt? > Do you mean this? > > if (mapping_flags & IOMAP_DAX) > iomap->dax_dev = target->bt_daxdev; > else > iomap->bdev = target->bt_bdev; > > The dax path wants dax_dev set so that it can do the glorified memcpy > operation, and it doesn't need (or want) a block device. Yes, so proper to use target->bt_bdev for checks for bdev atomic write capability, right? Thanks, John
On Wed, Feb 21, 2024 at 05:38:39PM +0000, John Garry wrote: > On 21/02/2024 17:00, Darrick J. Wong wrote: > > > > Hmm. Well, if we move towards pushing all the hardware checks out of > > > > xfs/iomap and into whatever goes on underneath submit_bio then I guess > > > > we don't need to check device support here at all. > > > Yeah, I have been thinking about this. But I was still planning on putting a > > > "bdev on atomic write" check here, as you mentioned. > > > > > > But is this a proper method to access the bdev for an xfs inode: > > > > > > STATIC bool > > > xfs_file_can_atomic_write( > > > struct xfs_inode *inode) > > > { > > > struct xfs_buftarg *target = xfs_inode_buftarg(inode); > > > struct block_device *bdev = target->bt_bdev; > > > > > > if (!xfs_inode_atomicwrites(inode)) > > > return false; > > > > > > return bdev_can_atomic_write(bdev); > > > } > > There's still a TOCTOU race problem if the bdev gets reconfigured > > between xfs_file_can_atomic_write and submit_bio. > > If that is the case then a check in the bio submit path is required to catch > any such reconfigure problems - and we effectively have that in this series. > > I am looking at change some of these XFS bdev_can_atomic_write() checks, but > would still have a check in the bio submit path. <nod> "check in the bio submit path" sounds good to me. Adding in redundant checks which are eventually gated on whatever submit_bio does sounds like excessive overhead and layering violations. > > > > However, if you're only using this to advertise the capability via statx > > then I suppose that's fine -- userspace has to have some means of > > discovering the ability at all. Userspace is also inherently racy. > > > > > I do notice the dax check in xfs_bmbt_to_iomap() when assigning iomap->bdev, > > > which is creating some doubt? > > Do you mean this? > > > > if (mapping_flags & IOMAP_DAX) > > iomap->dax_dev = target->bt_daxdev; > > else > > iomap->bdev = target->bt_bdev; > > > > The dax path wants dax_dev set so that it can do the glorified memcpy > > operation, and it doesn't need (or want) a block device. > > Yes, so proper to use target->bt_bdev for checks for bdev atomic write > capability, right? Right. fsdax doesn't support atomic memcpy to pmem. --D > > Thanks, > John > >
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index e33e5e13b95f..1375d0089806 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -1232,6 +1232,8 @@ xfs_file_open( return -EIO; file->f_mode |= FMODE_NOWAIT | FMODE_BUF_RASYNC | FMODE_BUF_WASYNC | FMODE_DIO_PARALLEL_WRITE | FMODE_CAN_ODIRECT; + if (xfs_inode_atomicwrites(XFS_I(inode))) + 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. Signed-off-by: John Garry <john.g.garry@oracle.com> --- fs/xfs/xfs_file.c | 2 ++ 1 file changed, 2 insertions(+)