| Message ID | 20170424132259.8680-12-jlayton@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Mon, Apr 24, 2017 at 09:22:50AM -0400, Jeff Layton wrote: > Signed-off-by: Jeff Layton <jlayton@redhat.com> > --- > fs/cifs/file.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/fs/cifs/file.c b/fs/cifs/file.c > index 21d404535739..4b696a23b0b1 100644 > --- a/fs/cifs/file.c > +++ b/fs/cifs/file.c > @@ -2234,14 +2234,16 @@ cifs_writepage_locked(struct page *page, struct writeback_control *wbc) > set_page_writeback(page); > retry_write: > rc = cifs_partialpagewrite(page, 0, PAGE_SIZE); > + if (rc == -EAGAIN && wbc->sync_mode == WB_SYNC_ALL) { > goto retry_write; > + } else if (rc == -EAGAIN) { > redirty_page_for_writepage(wbc, page); > + } else if (rc != 0) { > SetPageError(page); > + mapping_set_error(page->mapping, rc); > + } else { > SetPageUptodate(page); > + } Hmmm. I might be a little too nitpicky, but I hate having the same partial condition duplicated if possible. Why not: if (rc == -EAGAIN) { if (wbc->sync_mode == WB_SYNC_ALL) goto retry_write; redirty_page_for_writepage(wbc, page); } else if (rc) { SetPageError(page); mapping_set_error(page->mapping, rc); } else { SetPageUptodate(page); } Otherwise this looks fine to me: Reviewed-by: Christoph Hellwig <hch@lst.de> -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2017-04-24 at 08:27 -0700, Christoph Hellwig wrote: > On Mon, Apr 24, 2017 at 09:22:50AM -0400, Jeff Layton wrote: > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > > --- > > fs/cifs/file.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/fs/cifs/file.c b/fs/cifs/file.c > > index 21d404535739..4b696a23b0b1 100644 > > --- a/fs/cifs/file.c > > +++ b/fs/cifs/file.c > > @@ -2234,14 +2234,16 @@ cifs_writepage_locked(struct page *page, struct writeback_control *wbc) > > set_page_writeback(page); > > retry_write: > > rc = cifs_partialpagewrite(page, 0, PAGE_SIZE); > > + if (rc == -EAGAIN && wbc->sync_mode == WB_SYNC_ALL) { > > goto retry_write; > > + } else if (rc == -EAGAIN) { > > redirty_page_for_writepage(wbc, page); > > + } else if (rc != 0) { > > SetPageError(page); > > + mapping_set_error(page->mapping, rc); > > + } else { > > SetPageUptodate(page); > > + } > > Hmmm. I might be a little too nitpicky, but I hate having the same > partial condition duplicated if possible. Why not: > > if (rc == -EAGAIN) { > if (wbc->sync_mode == WB_SYNC_ALL) > goto retry_write; > redirty_page_for_writepage(wbc, page); > } else if (rc) { > SetPageError(page); > mapping_set_error(page->mapping, rc); > } else { > SetPageUptodate(page); > } > > Otherwise this looks fine to me: > > Reviewed-by: Christoph Hellwig <hch@lst.de> No, you're absolutely right. I merged that change into the patch. Thanks for the review so far!
diff --git a/fs/cifs/file.c b/fs/cifs/file.c index 21d404535739..4b696a23b0b1 100644 --- a/fs/cifs/file.c +++ b/fs/cifs/file.c @@ -2234,14 +2234,16 @@ cifs_writepage_locked(struct page *page, struct writeback_control *wbc) set_page_writeback(page); retry_write: rc = cifs_partialpagewrite(page, 0, PAGE_SIZE); - if (rc == -EAGAIN && wbc->sync_mode == WB_SYNC_ALL) + if (rc == -EAGAIN && wbc->sync_mode == WB_SYNC_ALL) { goto retry_write; - else if (rc == -EAGAIN) + } else if (rc == -EAGAIN) { redirty_page_for_writepage(wbc, page); - else if (rc != 0) + } else if (rc != 0) { SetPageError(page); - else + mapping_set_error(page->mapping, rc); + } else { SetPageUptodate(page); + } end_page_writeback(page); put_page(page); free_xid(xid);
Signed-off-by: Jeff Layton <jlayton@redhat.com> --- fs/cifs/file.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)