| Message ID | 7baf74b071f9d9002d2543cfc4f86bd3ddf7127f.1679910088.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: optimize disks flush code | expand |
On Mon, Mar 27, 2023 at 05:53:10PM +0800, Anand Jain wrote: > The function wait_dev_flush() tests for the BTRFS_DEV_STATE_FLUSH_SENT > bit and then clears it separately. Instead, use test_and_clear_bit(). But why would we need to do it like that? The write and wait are executed in one thread so we don't need atomic change to the status and thus a separate set/test/clear bit are fine. If not, then please explain. Thanks.
On 3/28/23 01:14, David Sterba wrote: > On Mon, Mar 27, 2023 at 05:53:10PM +0800, Anand Jain wrote: >> The function wait_dev_flush() tests for the BTRFS_DEV_STATE_FLUSH_SENT >> bit and then clears it separately. Instead, use test_and_clear_bit(). > > But why would we need to do it like that? The write and wait are > executed in one thread so we don't need atomic change to the status and > thus a separate set/test/clear bit are fine. If not, then please > explain. Thanks. It's true that atomic test_and_clear_bit() isn't necessary in this case. Nonetheless, using it have benefits such as cleaner code and improved efficiency[1]. [1]. I was curious, so I made wait_dev_flush() non-inline and checked the ASM code for wait_dev_flush(). After the patch, there were 8 fewer instructions. I'm okay with dropping this patch if you prefer to maintain the correct usage of test_and_clear_bit(). Thanks, Anand
On Tue, Mar 28, 2023 at 01:05:12PM +0800, Anand Jain wrote: > > > On 3/28/23 01:14, David Sterba wrote: > > On Mon, Mar 27, 2023 at 05:53:10PM +0800, Anand Jain wrote: > >> The function wait_dev_flush() tests for the BTRFS_DEV_STATE_FLUSH_SENT > >> bit and then clears it separately. Instead, use test_and_clear_bit(). > > > > But why would we need to do it like that? The write and wait are > > executed in one thread so we don't need atomic change to the status and > > thus a separate set/test/clear bit are fine. If not, then please > > explain. Thanks. > > It's true that atomic test_and_clear_bit() isn't necessary in this case. > Nonetheless, using it have benefits such as cleaner code and improved > efficiency[1]. > > [1]. I was curious, so I made wait_dev_flush() non-inline and checked > the ASM code for wait_dev_flush(). After the patch, there were 8 fewer > instructions. > > I'm okay with dropping this patch if you prefer to maintain the correct > usage of test_and_clear_bit(). Fewer instructions is a bonus here, but from the logic POV a test_bit in a condition immediately followed by a clear_bit is not a common pattern. So even if we don't need the atomic semantics it's following a common pattern which is good.
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 040142f2e76c..1f9e2a2a8267 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -4108,10 +4108,9 @@ static bool wait_dev_flush(struct btrfs_device *device) { struct bio *bio = &device->flush_bio; - if (!test_bit(BTRFS_DEV_STATE_FLUSH_SENT, &device->dev_state)) + if (!test_and_clear_bit(BTRFS_DEV_STATE_FLUSH_SENT, &device->dev_state)) return true; - clear_bit(BTRFS_DEV_STATE_FLUSH_SENT, &device->dev_state); wait_for_completion_io(&device->flush_wait); if (bio->bi_status) {
The function wait_dev_flush() tests for the BTRFS_DEV_STATE_FLUSH_SENT bit and then clears it separately. Instead, use test_and_clear_bit(). Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/disk-io.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)