| Message ID | 7c4779f1f0c8ead30f660a2cfbdf4d7cc08e405a.1729825985.git.ritesh.list@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ext4: Add atomic write support for DIO | expand |
On Fri, Oct 25, 2024 at 09:15:53AM +0530, Ritesh Harjani (IBM) wrote: > iomap will not return -ENOTBLK in case of dio atomic writes. But let's > also add a WARN_ON_ONCE and return -EIO as a safety net. > > Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> > --- > fs/ext4/file.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/fs/ext4/file.c b/fs/ext4/file.c > index f9516121a036..af6ebd0ac0d6 100644 > --- a/fs/ext4/file.c > +++ b/fs/ext4/file.c > @@ -576,8 +576,16 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from) > iomap_ops = &ext4_iomap_overwrite_ops; > ret = iomap_dio_rw(iocb, from, iomap_ops, &ext4_dio_write_ops, > dio_flags, NULL, 0); > - if (ret == -ENOTBLK) > + if (ret == -ENOTBLK) { > ret = 0; > + /* > + * iomap will never return -ENOTBLK if write fails for atomic > + * write. But let's just add a safety net. I think it can if the pagecache invalidation fails, so you really do need the safety net. I suspect that the xfs version of this series needs it too, though it may have fallen out? Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > + */ > + if (WARN_ON_ONCE(iocb->ki_flags & IOCB_ATOMIC)) > + ret = -EIO; > + } > + > if (extend) { > /* > * We always perform extending DIO write synchronously so by > -- > 2.46.0 > >
"Darrick J. Wong" <djwong@kernel.org> writes: > On Fri, Oct 25, 2024 at 09:15:53AM +0530, Ritesh Harjani (IBM) wrote: >> iomap will not return -ENOTBLK in case of dio atomic writes. But let's >> also add a WARN_ON_ONCE and return -EIO as a safety net. >> >> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> >> --- >> fs/ext4/file.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/fs/ext4/file.c b/fs/ext4/file.c >> index f9516121a036..af6ebd0ac0d6 100644 >> --- a/fs/ext4/file.c >> +++ b/fs/ext4/file.c >> @@ -576,8 +576,16 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from) >> iomap_ops = &ext4_iomap_overwrite_ops; >> ret = iomap_dio_rw(iocb, from, iomap_ops, &ext4_dio_write_ops, >> dio_flags, NULL, 0); >> - if (ret == -ENOTBLK) >> + if (ret == -ENOTBLK) { >> ret = 0; >> + /* >> + * iomap will never return -ENOTBLK if write fails for atomic >> + * write. But let's just add a safety net. > > I think it can if the pagecache invalidation fails, so you really do > need the safety net. Ah, right! So in that case I should remove WARN_ON_ONCE and correct the comment too. > I suspect that the xfs version of this series > needs it too, though it may have fallen out? > I think so yes. Looks like it got missed. > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > > --D > Thanks for pointing that out. -ritesh >> + */ >> + if (WARN_ON_ONCE(iocb->ki_flags & IOCB_ATOMIC)) >> + ret = -EIO; >> + } >> + >> if (extend) { >> /* >> * We always perform extending DIO write synchronously so by >> -- >> 2.46.0 >> >>
On Fri, Oct 25, 2024 at 09:15:53AM +0530, Ritesh Harjani (IBM) wrote: > iomap will not return -ENOTBLK in case of dio atomic writes. But let's > also add a WARN_ON_ONCE and return -EIO as a safety net. > > Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> > --- > fs/ext4/file.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/fs/ext4/file.c b/fs/ext4/file.c > index f9516121a036..af6ebd0ac0d6 100644 > --- a/fs/ext4/file.c > +++ b/fs/ext4/file.c > @@ -576,8 +576,16 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from) > iomap_ops = &ext4_iomap_overwrite_ops; > ret = iomap_dio_rw(iocb, from, iomap_ops, &ext4_dio_write_ops, > dio_flags, NULL, 0); > - if (ret == -ENOTBLK) > + if (ret == -ENOTBLK) { > ret = 0; > + /* > + * iomap will never return -ENOTBLK if write fails for atomic > + * write. But let's just add a safety net. > + */ > + if (WARN_ON_ONCE(iocb->ki_flags & IOCB_ATOMIC)) > + ret = -EIO; > + } Why can't the iomap code return EIO in this case for IOCB_ATOMIC? That way we don't have to put this logic into every filesystem. When/if we start supporting atomic writes for buffered IO, then it's worth pushing this out to filesystems, but right now it doesn't seem necessary... -Dave.
Hi Dave, Dave Chinner <david@fromorbit.com> writes: > On Fri, Oct 25, 2024 at 09:15:53AM +0530, Ritesh Harjani (IBM) wrote: >> iomap will not return -ENOTBLK in case of dio atomic writes. But let's >> also add a WARN_ON_ONCE and return -EIO as a safety net. >> >> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> >> --- >> fs/ext4/file.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/fs/ext4/file.c b/fs/ext4/file.c >> index f9516121a036..af6ebd0ac0d6 100644 >> --- a/fs/ext4/file.c >> +++ b/fs/ext4/file.c >> @@ -576,8 +576,16 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from) >> iomap_ops = &ext4_iomap_overwrite_ops; >> ret = iomap_dio_rw(iocb, from, iomap_ops, &ext4_dio_write_ops, >> dio_flags, NULL, 0); >> - if (ret == -ENOTBLK) >> + if (ret == -ENOTBLK) { >> ret = 0; >> + /* >> + * iomap will never return -ENOTBLK if write fails for atomic >> + * write. But let's just add a safety net. >> + */ >> + if (WARN_ON_ONCE(iocb->ki_flags & IOCB_ATOMIC)) >> + ret = -EIO; >> + } > > Why can't the iomap code return EIO in this case for IOCB_ATOMIC? > That way we don't have to put this logic into every filesystem. This was origially intended as a safety net hence the WARN_ON_ONCE. Later Darrick pointed out that we still might have an unconverted condition in iomap which can return ENOTBLK for DIO atomic writes (page cache invalidation). You pointed it right that it should be fixed in iomap. However do you think filesystems can still keep this as safety net (maybe no need of WARN_ON_ONCE). > > When/if we start supporting atomic writes for buffered IO, then it's > worth pushing this out to filesystems, but right now it doesn't seem > necessary... > > -Dave. > -- > Dave Chinner > david@fromorbit.com
diff --git a/fs/ext4/file.c b/fs/ext4/file.c index f9516121a036..af6ebd0ac0d6 100644 --- a/fs/ext4/file.c +++ b/fs/ext4/file.c @@ -576,8 +576,16 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from) iomap_ops = &ext4_iomap_overwrite_ops; ret = iomap_dio_rw(iocb, from, iomap_ops, &ext4_dio_write_ops, dio_flags, NULL, 0); - if (ret == -ENOTBLK) + if (ret == -ENOTBLK) { ret = 0; + /* + * iomap will never return -ENOTBLK if write fails for atomic + * write. But let's just add a safety net. + */ + if (WARN_ON_ONCE(iocb->ki_flags & IOCB_ATOMIC)) + ret = -EIO; + } + if (extend) { /* * We always perform extending DIO write synchronously so by
iomap will not return -ENOTBLK in case of dio atomic writes. But let's also add a WARN_ON_ONCE and return -EIO as a safety net. Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> --- fs/ext4/file.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)