2010/12/11 Jeff Layton <[email protected]>: > If CIFSSMBWrite2 returns -EAGAIN, then the error should be considered > temporary. CIFS should retry the write instead of setting an error on > the mapping and returning. > > For WB_SYNC_ALL, just retry the write immediately. In the WB_SYNC_NONE > case, call redirty_page_for_writeback on all of the pages that didn't > get written out and then move on. > > Also, fix up the handling of a short write with a successful return > code. MS-CIFS says that 0 bytes_written means ENOSPC or EFBIG. It > doesn't mention what a short, but non-zero write means, so for now > treat it as we would an -EAGAIN return. > > Signed-off-by: Jeff Layton <[email protected]> > --- > fs/cifs/file.c | 48 ++++++++++++++++++++++++++++++++++++------------ > 1 files changed, 36 insertions(+), 12 deletions(-) > > diff --git a/fs/cifs/file.c b/fs/cifs/file.c > index fe16f6d..8bef611 100644 > --- a/fs/cifs/file.c > +++ b/fs/cifs/file.c > @@ -1430,6 +1430,7 @@ retry: > break; > } > if (n_iov) { > +retry_write: > open_file = find_writable_file(CIFS_I(mapping->host), > false); > if (!open_file) { > @@ -1442,31 +1443,54 @@ retry: > &bytes_written, iov, n_iov, > long_op); > cifsFileInfo_put(open_file); > - cifs_update_eof(cifsi, offset, bytes_written); > } > > - if (rc || bytes_written < bytes_to_write) { > - cERROR(1, "Write2 ret %d, wrote %d", > - rc, bytes_written); > - mapping_set_error(mapping, rc); > - } else { > + cFYI(1, "Write2 rc=%d, wrote=%u", rc, bytes_written); > + > + /* > + * For now, treat a short write as if nothing got > + * written. A zero length write however indicates > + * ENOSPC or EFBIG. We have no way to know which > + * though, so call it ENOSPC for now. EFBIG would > + * get translated to AS_EIO anyway. > + * > + * FIXME: make it take into account the data that did > + * get written > + */ > + if (rc == 0) { > + if (bytes_written == 0) > + rc = -ENOSPC; > + else if (bytes_written < bytes_to_write) > + rc = -EAGAIN; > + } > + > + /* retry on data-integrity flush */ > + if (wbc->sync_mode == WB_SYNC_ALL && > + (rc == -EAGAIN || bytes_written < bytes_to_write)) > + goto retry_write; > + > + /* fix the stats and EOF */ > + if (bytes_written > 0) { > cifs_stats_bytes_written(tcon, bytes_written); > + cifs_update_eof(cifsi, offset, bytes_written); > } > > for (i = 0; i < n_iov; i++) { > page = pvec.pages[first + i]; > - /* Should we also set page error on > - success rc but too little data written? */ > - /* BB investigate retry logic on temporary > - server crash cases and how recovery works > - when page marked as error */ > - if (rc) > + /* on retryable write error, redirty page */ > + if (rc == -EAGAIN) > + redirty_page_for_writepage(wbc, page); > + else if (rc != 0) > SetPageError(page); > kunmap(page); > unlock_page(page); > end_page_writeback(page); > page_cache_release(page); > } > + > + if (rc != -EAGAIN) > + mapping_set_error(mapping, rc); > + > if ((wbc->nr_to_write -= n_iov) <= 0) > done = 1; > index = next; > -- > 1.7.3.2
Looks good. Reviewed-by: Pavel Shilovsky <[email protected]> I also think that not retrying on EAGAIN here can cause a error on smb2 code (sometimes -one or two times for ten runs - I got file with a size that was less than 400MB while I wrote exactly 400MB during my test). -- Best regards, Pavel Shilovsky.
