| Message ID | 20231126124720.1249310-3-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/13] iomap: clear the per-folio dirty bits on all writeback failures | expand |
Christoph Hellwig <hch@lst.de> writes: > iomap_writepage_map aready warns about inline data, but then just ignores > it. Treat it as an error and return -EIO. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/iomap/buffered-io.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) The code change looks very obvious. But sorry that I have some queries which I would like to clarify - The dirty page we are trying to write can always belong to the dirty inode with inline data in it right? So it is then the FS responsibility to un-inline the inode in the ->map_blocks call is it? Looking at the gfs2 code, it might as well return iomap->type as IOMAP_INLINE for IOMAP_WRITE request in it's iomap_writeback_ops no? iomap_writeback_ops -> gfs2_map_blocks -> __gfs2_iomap_get > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index 98d52feb220f0a..b1bcc43baf0caf 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -1818,8 +1818,10 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc, > if (error) > break; > trace_iomap_writepage_map(inode, &wpc->iomap); > - if (WARN_ON_ONCE(wpc->iomap.type == IOMAP_INLINE)) > - continue; > + if (WARN_ON_ONCE(wpc->iomap.type == IOMAP_INLINE)) { > + error = -EIO; > + break; > + } > if (wpc->iomap.type == IOMAP_HOLE) > continue; > iomap_add_to_ioend(inode, pos, folio, ifs, wpc, wbc, > -- > 2.39.2 -ritesh
On Mon, Nov 27, 2023 at 10:31:31AM +0530, Ritesh Harjani wrote: > The code change looks very obvious. But sorry that I have some queries > which I would like to clarify - > > The dirty page we are trying to write can always belong to the dirty > inode with inline data in it right? Yes. > So it is then the FS responsibility to un-inline the inode in the > ->map_blocks call is it? I think they way it currently works for gfs2 is that writeback from the page cache never goes back into the inline area. If we ever have a need to actually write back inline data we could change this code to support it, but right now I just want to make the assert more consistent.
On Mon, Nov 27, 2023 at 07:33:25AM +0100, Christoph Hellwig wrote: > On Mon, Nov 27, 2023 at 10:31:31AM +0530, Ritesh Harjani wrote: > > The code change looks very obvious. But sorry that I have some queries > > which I would like to clarify - > > > > The dirty page we are trying to write can always belong to the dirty > > inode with inline data in it right? > > Yes. > > > So it is then the FS responsibility to un-inline the inode in the > > ->map_blocks call is it? > > I think they way it currently works for gfs2 is that writeback from the > page cache never goes back into the inline area. > > If we ever have a need to actually write back inline data we could > change this code to support it, but right now I just want to make the > assert more consistent. Question: Do we even /want/ writeback to be initiating transactions to log the inline data? I suppose for ext4/jbd2 that would be the least inefficient time to do that. --D
On Tue, Nov 28, 2023 at 08:40:57PM -0800, Darrick J. Wong wrote: > > I think they way it currently works for gfs2 is that writeback from the > > page cache never goes back into the inline area. > > > > If we ever have a need to actually write back inline data we could > > change this code to support it, but right now I just want to make the > > assert more consistent. > > Question: Do we even /want/ writeback to be initiating transactions > to log the inline data? I suppose for ext4/jbd2 that would be the least > inefficient time to do that. I think in general not, but it really depends on the file system architecture. In other words: don't touch the current behavior unless we have a good reason. I just want to make the error check a little saner..
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index 98d52feb220f0a..b1bcc43baf0caf 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -1818,8 +1818,10 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc, if (error) break; trace_iomap_writepage_map(inode, &wpc->iomap); - if (WARN_ON_ONCE(wpc->iomap.type == IOMAP_INLINE)) - continue; + if (WARN_ON_ONCE(wpc->iomap.type == IOMAP_INLINE)) { + error = -EIO; + break; + } if (wpc->iomap.type == IOMAP_HOLE) continue; iomap_add_to_ioend(inode, pos, folio, ifs, wpc, wbc,
iomap_writepage_map aready warns about inline data, but then just ignores it. Treat it as an error and return -EIO. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/iomap/buffered-io.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)