Re: [PATCH v4 5/6] Revert "ceph: SetPageError() for writeback pages if writepages fails"

[Jeff Layton <jlayton@redhat.com>]

On Fri, 2017-02-10 at 19:22 +0800, Yan, Zheng wrote:
> > On 9 Feb 2017, at 22:48, Jeff Layton <jlayton@redhat.com> wrote:
> > 
> > This reverts commit b109eec6f4332bd517e2f41e207037c4b9065094.
> > 
> > If I'm filling up a filesystem with this sort of command:
> > 
> >    $ dd if=/dev/urandom of=/mnt/cephfs/fillfile bs=2M oflag=sync
> > 
> > ...then I'll eventually get back EIO on a write. Further calls
> > will give us ENOSPC.
> > 
> > I'm not sure what prompted this change, but I don't think it's what we
> > want to do. If writepages failed, we will have already set the mapping
> > error appropriately, and that's what gets reported by fsync() or
> > close().
> > 
> > __filemap_fdatawait_range however, does this:
> > 
> > 	wait_on_page_writeback(page);
> > 	if (TestClearPageError(page))
> > 		ret = -EIO;
> > 
> > ...and that -EIO ends up trumping the mapping's error if one exists.
> > 
> > When writepages fails, we only want to set the error in the mapping,
> > and not flag the individual pages.
> 
> I think you are right. Maybe we should also remove the SetPageError in writepage_nounlock
> 

Yeah, good catch. The last patch in this series should probably also
call ceph_set/clear_error_write in that function as well.

> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> > fs/ceph/addr.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> > index 308787eeee2c..040d05c8f4a2 100644
> > --- a/fs/ceph/addr.c
> > +++ b/fs/ceph/addr.c
> > @@ -668,6 +668,7 @@ static void writepages_finish(struct ceph_osd_request *req)
> > 	struct ceph_fs_client *fsc = ceph_inode_to_client(inode);
> > 	bool remove_page;
> > 
> > +
> > 	dout("writepages_finish %p rc %d\n", inode, rc);
> > 	if (rc < 0)
> > 		mapping_set_error(mapping, rc);
> > @@ -702,9 +703,6 @@ static void writepages_finish(struct ceph_osd_request *req)
> > 				clear_bdi_congested(&fsc->backing_dev_info,
> > 						    BLK_RW_ASYNC);
> > 
> > -			if (rc < 0)
> > -				SetPageError(page);
> > -
> > 			ceph_put_snap_context(page_snap_context(page));
> > 			page->private = 0;
> > 			ClearPagePrivate(page);
> > -- 
> > 2.9.3
> > 
> 
> 

-- 
Jeff Layton <jlayton@redhat.com>