On Mon, Sep 16, 2019 at 05:05:33AM -0700, Christoph Hellwig wrote:
> On Thu, Sep 12, 2019 at 09:04:30PM +1000, Matthew Bobrowski wrote:
> > @@ -3581,10 +3581,21 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
> > iomap->type = delalloc ? IOMAP_DELALLOC : IOMAP_HOLE;
> > iomap->addr = IOMAP_NULL_ADDR;
> > } else {
> > - if (map.m_flags & EXT4_MAP_MAPPED) {
> > - iomap->type = IOMAP_MAPPED;
> > - } else if (map.m_flags & EXT4_MAP_UNWRITTEN) {
> > + /*
> > + * Flags passed to ext4_map_blocks() for direct IO
> > + * writes can result in m_flags having both
> > + * EXT4_MAP_MAPPED and EXT4_MAP_UNWRITTEN bits set. In
> > + * order for allocated unwritten extents to be
> > + * converted to written extents in the end_io handler
> > + * correctly, we need to ensure that the iomap->type
> > + * is also set appropriately in that case. Thus, we
> > + * need to check whether EXT4_MAP_UNWRITTEN is set
> > + * first.
> > + */
> > + if (map.m_flags & EXT4_MAP_UNWRITTEN) {
> > iomap->type = IOMAP_UNWRITTEN;
> > + } else if (map.m_flags & EXT4_MAP_MAPPED) {
> > + iomap->type = IOMAP_MAPPED;
>
> I think much of this would benefit a lot from just being split up.
> I hacked up a patch last week that split the ext4 direct I/O code
> a bit, but this is completely untested and needs further splitup,
> but maybe you can take it as an inspiration for your series?
Nice, I really like this! :-)
The ext4_iomap_begin() callback is kind of already getting larger than it
should have to be and I can only see it growing moving forward, so why not
split it up now.
> E.g. at least one helper for filling out the iomap from the ext4
> map data, and one for the seek unwritten extent reporting. The
> split of the overall iomap ops seemed useful to me, but might not
> be as important with the other cleanups:
Yeah, I think I'll leave the iomap operations as they are for now. Something
to definitely consider at a later point though.
--<M>--
