| Message ID | 20170209144836.12525-6-jlayton@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> 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 Regards Yan, Zheng > > 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 > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > > > >
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);
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. Signed-off-by: Jeff Layton <jlayton@redhat.com> --- fs/ceph/addr.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)