2010/12/11 Jeff Layton <jlayton@xxxxxxxxxx>:
> 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 <jlayton@xxxxxxxxxx>
> ---
> 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 <piastryyy@xxxxxxxxx>
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.
ÿôèº{.nÇ+?·?®??+%?Ëÿ±éݶ¥?wÿº{.nÇ+?·¥?{±ýÈ?³ø§¶?¡Ü¨}©?²Æ zÚ&j:+v?¨þø ¯ù ®w¥þ?à2?Þ?¨èÚ&¢)ß¡«a¶Úÿÿûàz¿äz¹Þ?ú+?ù???Ý¢jÿ?wèþf
