On 18:37 05/09, Christoph Hellwig wrote:
> On Thu, Sep 05, 2019 at 10:06:37AM -0500, Goldwyn Rodrigues wrote:
> > - else if (iomap->flags & IOMAP_F_BUFFER_HEAD)
> > + } else if (iomap->flags & IOMAP_F_COW) {
> > + if (WARN_ON_ONCE(iomap->flags & IOMAP_F_BUFFER_HEAD)) {
> > + status = -EIO;
> > + goto out_no_page;
> > + }
> > + if (WARN_ON_ONCE(srcmap->type == IOMAP_HOLE &&
> > + srcmap->addr != IOMAP_NULL_ADDR)) {
>
> Well, we want HOLES to have IOMAP_NULL_ADDR everywhere, so not sure
> why the assert is just here.
This came up as one of the review comments for checking srcmap.
This does look ugly after taking out iomap_assert(). Removing.
>
> > + status = -EIO;
> > + goto out_no_page;
> > + }
> > + status = __iomap_write_begin(inode, pos, len, page, srcmap);
> > + } else if (iomap->flags & IOMAP_F_BUFFER_HEAD) {
> > status = __block_write_begin_int(page, pos, len, NULL, iomap);
> > - else
> > + } else {
> > status = __iomap_write_begin(inode, pos, len, page, iomap);
> > + }
>
> Maybe a good way to structure this is:
>
> if (iomap->flags & IOMAP_F_BUFFER_HEAD) {
> if (WARN_ON_ONCE(iomap->flags & IOMAP_F_COW)) {
> status = -EIO;
> goto out_no_page;
> }
> status = __block_write_begin_int(page, pos, len, NULL, iomap);
> } else {
> status = __iomap_write_begin(inode, pos, len, page,
> (iomap->flags & IOMAP_F_COW) ? srcmap : iomap);
> }
Yes, this looks much better. Will incorporate.
--
Goldwyn
