qemu-commits
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Qemu-commits] [qemu/qemu] 3817f1: block/nbd: Delete reconnect delay tim


From: Peter Maydell
Subject: [Qemu-commits] [qemu/qemu] 3817f1: block/nbd: Delete reconnect delay timer when done
Date: Wed, 09 Feb 2022 06:59:16 -0800

  Branch: refs/heads/staging
  Home:   https://github.com/qemu/qemu
  Commit: 3817f1daaca4b7bcc1f2e11446eb5fc2d6c4ed06
      
https://github.com/qemu/qemu/commit/3817f1daaca4b7bcc1f2e11446eb5fc2d6c4ed06
  Author: Hanna Reitz <hreitz@redhat.com>
  Date:   2022-02-09 (Wed, 09 Feb 2022)

  Changed paths:
    M block/nbd.c

  Log Message:
  -----------
  block/nbd: Delete reconnect delay timer when done

We start the reconnect delay timer to cancel the reconnection attempt
after a while.  Once nbd_co_do_establish_connection() has returned, this
attempt is over, and we no longer need the timer.

Delete it before returning from nbd_reconnect_attempt(), so that it does
not persist beyond the I/O request that was paused for reconnecting; we
do not want it to fire in a drained section, because all sort of things
can happen in such a section (e.g. the AioContext might be changed, and
we do not want the timer to fire in the wrong context; or the BDS might
even be deleted, and so the timer CB would access already-freed data).

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Hanna Reitz <hreitz@redhat.com>


  Commit: c99bf825452476469720c22eca1afd1327afd3fd
      
https://github.com/qemu/qemu/commit/c99bf825452476469720c22eca1afd1327afd3fd
  Author: Hanna Reitz <hreitz@redhat.com>
  Date:   2022-02-09 (Wed, 09 Feb 2022)

  Changed paths:
    M block/nbd.c

  Log Message:
  -----------
  block/nbd: Delete open timer when done

We start the open timer to cancel the connection attempt after a while.
Once nbd_do_establish_connection() has returned, the attempt is over,
and we no longer need the timer.

Delete it before returning from nbd_open(), so that it does not persist
for longer.  It has no use after nbd_open(), and just like the reconnect
delay timer, it might well be dangerous if it were to fire afterwards.

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Hanna Reitz <hreitz@redhat.com>


  Commit: 97f62cc17610705d9b50648477c7eb1ea81d0b76
      
https://github.com/qemu/qemu/commit/97f62cc17610705d9b50648477c7eb1ea81d0b76
  Author: Hanna Reitz <hreitz@redhat.com>
  Date:   2022-02-09 (Wed, 09 Feb 2022)

  Changed paths:
    M block/nbd.c

  Log Message:
  -----------
  block/nbd: Assert there are no timers when closed

Our two timers must not remain armed beyond nbd_clear_bdrvstate(), or
they will access freed data when they fire.

This patch is separate from the patches that actually fix the issue
(HEAD^^ and HEAD^) so that you can run the associated regression iotest
(281) on a configuration that reproducibly exposes the bug.

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Hanna Reitz <hreitz@redhat.com>


  Commit: 02065b394ccd73d7d875e89fa604ba3ea8f16717
      
https://github.com/qemu/qemu/commit/02065b394ccd73d7d875e89fa604ba3ea8f16717
  Author: Hanna Reitz <hreitz@redhat.com>
  Date:   2022-02-09 (Wed, 09 Feb 2022)

  Changed paths:
    M tests/qemu-iotests/iotests.py

  Log Message:
  -----------
  iotests.py: Add QemuStorageDaemon class

This is a rather simple class that allows creating a QSD instance
running in the background and stopping it when no longer needed.

The __del__ handler is a safety net for when something goes so wrong in
a test that e.g. the tearDown() method is not called (e.g. setUp()
launches the QSD, but then launching a VM fails).  We do not want the
QSD to continue running after the test has failed, so __del__() will
take care to kill it.

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Hanna Reitz <hreitz@redhat.com>


  Commit: 99fcaa160b3a53219005212f4cb1910999537463
      
https://github.com/qemu/qemu/commit/99fcaa160b3a53219005212f4cb1910999537463
  Author: Hanna Reitz <hreitz@redhat.com>
  Date:   2022-02-09 (Wed, 09 Feb 2022)

  Changed paths:
    M tests/qemu-iotests/281
    M tests/qemu-iotests/281.out

  Log Message:
  -----------
  iotests/281: Test lingering timers

Prior to "block/nbd: Delete reconnect delay timer when done" and
"block/nbd: Delete open timer when done", both of those timers would
remain scheduled even after successfully (re-)connecting to the server,
and they would not even be deleted when the BDS is deleted.

This test constructs exactly this situation:
(1) Configure an @open-timeout, so the open timer is armed, and
(2) Configure a @reconnect-delay and trigger a reconnect situation
    (which succeeds immediately), so the reconnect delay timer is armed.
Then we immediately delete the BDS, and sleep for longer than the
@open-timeout and @reconnect-delay.  Prior to said patches, this caused
one (or both) of the timer CBs to access already-freed data.

Accessing freed data may or may not crash, so this test can produce
false successes, but I do not know how to show the problem in a better
or more reliable way.  If you run this test on "block/nbd: Assert there
are no timers when closed" and without the fix patches mentioned above,
you should reliably see an assertion failure.
(But all other tests that use the reconnect delay timer (264 and 277)
will fail in that configuration, too; as will nbd-reconnect-on-open,
which uses the open timer.)

Remove this test from the quick group because of the two second sleep
this patch introduces.

(I decided to put this test case into 281, because the main bug this
series addresses is in the interaction of the NBD block driver and I/O
threads, which is precisely the scope of 281.  The test case for that
other bug will also be put into the test class added here.

Also, excuse the test class's name, I couldn't come up with anything
better.  The "yield" part will make sense two patches from now.)

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Hanna Reitz <hreitz@redhat.com>


  Commit: bb19eba568c53b35c48e2b5401aa5faa02b25e00
      
https://github.com/qemu/qemu/commit/bb19eba568c53b35c48e2b5401aa5faa02b25e00
  Author: Hanna Reitz <hreitz@redhat.com>
  Date:   2022-02-09 (Wed, 09 Feb 2022)

  Changed paths:
    M block/nbd.c

  Log Message:
  -----------
  block/nbd: Move s->ioc on AioContext change

s->ioc must always be attached to the NBD node's AioContext.  If that
context changes, s->ioc must be attached to the new context.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2033626
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Hanna Reitz <hreitz@redhat.com>


  Commit: 1bd4523c2ded28b7805b971b9d3d746beabd0a94
      
https://github.com/qemu/qemu/commit/1bd4523c2ded28b7805b971b9d3d746beabd0a94
  Author: Hanna Reitz <hreitz@redhat.com>
  Date:   2022-02-09 (Wed, 09 Feb 2022)

  Changed paths:
    M tests/qemu-iotests/281
    M tests/qemu-iotests/281.out

  Log Message:
  -----------
  iotests/281: Let NBD connection yield in iothread

Put an NBD block device into an I/O thread, and then read data from it,
hoping that the NBD connection will yield during that read.  When it
does, the coroutine must be reentered in the block device's I/O thread,
which will only happen if the NBD block driver attaches the connection's
QIOChannel to the new AioContext.  It did not do that after 4ddb5d2fde
("block/nbd: drop connection_co") and prior to "block/nbd: Move s->ioc
on AioContext change", which would cause an assertion failure.

To improve our chances of yielding, the NBD server is throttled to
reading 64 kB/s, and the NBD client reads 128 kB, so it should yield at
some point.

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Hanna Reitz <hreitz@redhat.com>


  Commit: ab6079e9c7ab65da0f5c72d1e768080a8a207591
      
https://github.com/qemu/qemu/commit/ab6079e9c7ab65da0f5c72d1e768080a8a207591
  Author: Peter Maydell <peter.maydell@linaro.org>
  Date:   2022-02-09 (Wed, 09 Feb 2022)

  Changed paths:
    M block/nbd.c
    M tests/qemu-iotests/281
    M tests/qemu-iotests/281.out
    M tests/qemu-iotests/iotests.py

  Log Message:
  -----------
  Merge remote-tracking branch 'remotes/vsementsov/tags/pull-nbd-2022-02-09' 
into staging

nbd: handle AioContext change correctly

# gpg: Signature made Wed 09 Feb 2022 13:54:54 GMT
# gpg:                using RSA key 8B9C26CDB2FD147C880E86A1561F24C1F19F79FB
# gpg: Good signature from "Vladimir Sementsov-Ogievskiy 
<vsementsov@virtuozzo.com>" [unknown]
# gpg: WARNING: This key is not certified with a trusted signature!
# gpg:          There is no indication that the signature belongs to the owner.
# Primary key fingerprint: 8B9C 26CD B2FD 147C 880E  86A1 561F 24C1 F19F 79FB

* remotes/vsementsov/tags/pull-nbd-2022-02-09:
  iotests/281: Let NBD connection yield in iothread
  block/nbd: Move s->ioc on AioContext change
  iotests/281: Test lingering timers
  iotests.py: Add QemuStorageDaemon class
  block/nbd: Assert there are no timers when closed
  block/nbd: Delete open timer when done
  block/nbd: Delete reconnect delay timer when done

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


Compare: https://github.com/qemu/qemu/compare/0a301624c2f4...ab6079e9c7ab



reply via email to

[Prev in Thread] Current Thread [Next in Thread]