[Qemu-commits] [qemu/qemu] be2ebc: raw-posix: Fix comment for raw_co_get_block_status...

[GitHub]

  Branch: refs/heads/master
  Home:   https://github.com/qemu/qemu
  Commit: be2ebc6dad31918e9b6ba241dbe1ea0942c5d691
      
https://github.com/qemu/qemu/commit/be2ebc6dad31918e9b6ba241dbe1ea0942c5d691
  Author: Markus Armbruster <address@hidden>
  Date:   2014-11-18 (Tue, 18 Nov 2014)

  Changed paths:
    M block/raw-posix.c

  Log Message:
  -----------
  raw-posix: Fix comment for raw_co_get_block_status()

Missed in commit 705be72.

Signed-off-by: Markus Armbruster <address@hidden>
Reviewed-by: Paolo Bonzini <address@hidden>
Reviewed-by: Fam Zheng <address@hidden>
Reviewed-by: Eric Blake <address@hidden>
Reviewed-by: Max Reitz <address@hidden>
Signed-off-by: Max Reitz <address@hidden>


  Commit: c4875e5b2216cf5427459e619b10f75083565792
      
https://github.com/qemu/qemu/commit/c4875e5b2216cf5427459e619b10f75083565792
  Author: Markus Armbruster <address@hidden>
  Date:   2014-11-18 (Tue, 18 Nov 2014)

  Changed paths:
    M block/raw-posix.c

  Log Message:
  -----------
  raw-posix: SEEK_HOLE suffices, get rid of FIEMAP

Commit 5500316 (May 2012) implemented raw_co_is_allocated() as
follows:

1. If defined(CONFIG_FIEMAP), use the FS_IOC_FIEMAP ioctl

2. Else if defined(SEEK_HOLE) && defined(SEEK_DATA), use lseek()

3. Else pretend there are no holes

Later on, raw_co_is_allocated() was generalized to
raw_co_get_block_status().

Commit 4f11aa8 (May 2014) changed it to try the three methods in order
until success, because "there may be implementations which support
[SEEK_HOLE/SEEK_DATA] but not [FIEMAP] (e.g., NFSv4.2) as well as vice
versa."

Unfortunately, we used FIEMAP incorrectly: we lacked FIEMAP_FLAG_SYNC.
Commit 38c4d0a (Sep 2014) added it.  Because that's a significant
speed hit, the next commit 7c159037 put SEEK_HOLE/SEEK_DATA first.

As you see, the obvious use of FIEMAP is wrong, and the correct use is
slow.  I guess this puts it somewhere between -7 "The obvious use is
wrong" and -10 "It's impossible to get right" on Rusty Russel's Hard
to Misuse scale[*].

"Fortunately", the FIEMAP code is used only when

* SEEK_HOLE/SEEK_DATA aren't defined, but CONFIG_FIEMAP is

  Uncommon.  SEEK_HOLE had no XFS implementation between 2011 (when it
  was introduced for ext4 and btrfs) and 2012.

* SEEK_HOLE/SEEK_DATA and CONFIG_FIEMAP are defined, but lseek() fails

  Unlikely.

Thus, the FIEMAP code executes rarely.  Makes it a nice hidey-hole for
bugs.  Worse, bugs hiding there can theoretically bite even on a host
that has SEEK_HOLE/SEEK_DATA.

I don't want to worry about this crap, not even theoretically.  Get
rid of it.

[*] http://ozlabs.org/~rusty/index.cgi/tech/2008-04-01.html

Signed-off-by: Markus Armbruster <address@hidden>
Reviewed-by: Max Reitz <address@hidden>
Reviewed-by: Eric Blake <address@hidden>
Signed-off-by: Max Reitz <address@hidden>


  Commit: d1f06fe665acdd7aa7a46a5ef88172c3d7d3028e
      
https://github.com/qemu/qemu/commit/d1f06fe665acdd7aa7a46a5ef88172c3d7d3028e
  Author: Markus Armbruster <address@hidden>
  Date:   2014-11-18 (Tue, 18 Nov 2014)

  Changed paths:
    M block/raw-posix.c

  Log Message:
  -----------
  raw-posix: The SEEK_HOLE code is flawed, rewrite it

On systems where SEEK_HOLE in a trailing hole seeks to EOF (Solaris,
but not Linux), try_seek_hole() reports trailing data instead.

Additionally, unlikely lseek() failures are treated badly:

* When SEEK_HOLE fails, try_seek_hole() reports trailing data.  For
  -ENXIO, there's in fact a trailing hole.  Can happen only when
  something truncated the file since we opened it.

* When SEEK_HOLE succeeds, SEEK_DATA fails, and SEEK_END succeeds,
  then try_seek_hole() reports a trailing hole.  This is okay only
  when SEEK_DATA failed with -ENXIO (which means the non-trailing hole
  found by SEEK_HOLE has since become trailing somehow).  For other
  failures (unlikely), it's wrong.

* When SEEK_HOLE succeeds, SEEK_DATA fails, SEEK_END fails (unlikely),
  then try_seek_hole() reports bogus data [-1,start), which its caller
  raw_co_get_block_status() turns into zero sectors of data.  Could
  theoretically lead to infinite loops in code that attempts to scan
  data vs. hole forward.

Rewrite from scratch, with very careful comments.

Signed-off-by: Markus Armbruster <address@hidden>
Reviewed-by: Max Reitz <address@hidden>
Reviewed-by: Eric Blake <address@hidden>
Signed-off-by: Max Reitz <address@hidden>


  Commit: 867678530240ed7a4aaf647df08be98bebd3b1f0
      
https://github.com/qemu/qemu/commit/867678530240ed7a4aaf647df08be98bebd3b1f0
  Author: Kevin Wolf <address@hidden>
  Date:   2014-11-18 (Tue, 18 Nov 2014)

  Changed paths:
    M block/raw-posix.c

  Log Message:
  -----------
  Merge remote-tracking branch 'mreitz/block' into queue-block

* mreitz/block:
  raw-posix: The SEEK_HOLE code is flawed, rewrite it
  raw-posix: SEEK_HOLE suffices, get rid of FIEMAP
  raw-posix: Fix comment for raw_co_get_block_status()


  Commit: 39411cf3c316de0fe3cbb9585774bacfe3bd8efd
      
https://github.com/qemu/qemu/commit/39411cf3c316de0fe3cbb9585774bacfe3bd8efd
  Author: Max Reitz <address@hidden>
  Date:   2014-11-18 (Tue, 18 Nov 2014)

  Changed paths:
    M block/raw-posix.c

  Log Message:
  -----------
  block/raw-posix: Fix preallocating write() loop

write() may write less bytes than requested; in this case, the number of
bytes written is returned. This is the byte count we should be
subtracting from the number of bytes still to be written, and not the
byte count we requested to write.

Reported-by: László Érsek <address@hidden>
Signed-off-by: Max Reitz <address@hidden>
Signed-off-by: Kevin Wolf <address@hidden>


  Commit: 731de38052b245eab79e417aeac5e1dcebe6437f
      
https://github.com/qemu/qemu/commit/731de38052b245eab79e417aeac5e1dcebe6437f
  Author: Max Reitz <address@hidden>
  Date:   2014-11-18 (Tue, 18 Nov 2014)

  Changed paths:
    M block/raw-posix.c

  Log Message:
  -----------
  block/raw-posix: Only sync after successful preallocation

The loop which filled the file with zeroes may have been left early due
to an error. In that case, the fsync() should be skipped.

Signed-off-by: Max Reitz <address@hidden>
Signed-off-by: Kevin Wolf <address@hidden>


  Commit: 098ffa6674a82ceac0e3ccb3a8a5bf6ca44adcd5
      
https://github.com/qemu/qemu/commit/098ffa6674a82ceac0e3ccb3a8a5bf6ca44adcd5
  Author: Max Reitz <address@hidden>
  Date:   2014-11-18 (Tue, 18 Nov 2014)

  Changed paths:
    M block/raw-posix.c

  Log Message:
  -----------
  block/raw-posix: Catch fsync() errors

fsync() may fail, and that case should be handled.

Reported-by: László Érsek <address@hidden>
Signed-off-by: Max Reitz <address@hidden>
Signed-off-by: Kevin Wolf <address@hidden>


  Commit: 1ab8f867ef2ef0aa259e0e0d1040d67350af1bec
      
https://github.com/qemu/qemu/commit/1ab8f867ef2ef0aa259e0e0d1040d67350af1bec
  Author: Peter Maydell <address@hidden>
  Date:   2014-11-18 (Tue, 18 Nov 2014)

  Changed paths:
    M block/raw-posix.c

  Log Message:
  -----------
  Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging

Block patches for 2.2.0-rc2

# gpg: Signature made Tue 18 Nov 2014 11:32:55 GMT using RSA key ID C88F2FD6
# gpg: Good signature from "Kevin Wolf <address@hidden>"

* remotes/kevin/tags/for-upstream:
  block/raw-posix: Catch fsync() errors
  block/raw-posix: Only sync after successful preallocation
  block/raw-posix: Fix preallocating write() loop
  raw-posix: The SEEK_HOLE code is flawed, rewrite it
  raw-posix: SEEK_HOLE suffices, get rid of FIEMAP
  raw-posix: Fix comment for raw_co_get_block_status()

Signed-off-by: Peter Maydell <address@hidden>


Compare: https://github.com/qemu/qemu/compare/ea5b201a0acf...1ab8f867ef2e