All of lore.kernel.org
 help / color / mirror / Atom feed
From: Bart Van Assche <bart.vanassche@sandisk.com>
To: Mikulas Patocka <mpatocka@redhat.com>
Cc: Jens Axboe <axboe@kernel.dk>,
	Christoph Hellwig <hch@infradead.org>,
	"Mike Snitzer" <snitzer@redhat.com>,
	<linux-block@vger.kernel.org>, <dm-devel@redhat.com>,
	"Alasdair G. Kergon" <agk@redhat.com>
Subject: Re: [dm-devel] [PATCH 1/4] brd: handle misaligned discard
Date: Fri, 28 Oct 2016 08:55:26 -0700	[thread overview]
Message-ID: <f3257790-19a5-8512-df37-78e5fd8a8973@sandisk.com> (raw)
In-Reply-To: <alpine.LRH.2.02.1610280731410.22676@file01.intranet.prod.int.rdu2.redhat.com>

On 10/28/2016 04:39 AM, Mikulas Patocka wrote:
> On Wed, 26 Oct 2016, Bart Van Assche wrote:
>> On 10/26/2016 02:46 PM, Mikulas Patocka wrote:
>>> I think the proper thing would be to move "discard_zeroes_data" flag into
>>> the bio itself - there would be REQ_OP_DISCARD and REQ_OP_DISCARD_ZERO -
>>> and if the device doesn't support REQ_OP_DISCARD_ZERO, it rejects the bio
>>> and the caller is supposed to do zeroing manually.
>>
>> Sorry but I don't like this proposal. I think that a much better solution
>> would be to pause I/O before making any changes to the discard_zeroes_data
>> flag.
>
> The device mapper pauses all bios when it switches the table - but the bio
> was submitted with assumption that it goes to a device withe
> "discard_zeroes_data" set, then the bio is paused, device mapper table is
> switched, the bio is unpaused, and now it can go to a device without
> "discard_zeroes_data".

Hello Mikulas,

Sorry if I wasn't clear enough. What I meant is to wait until all 
outstanding requests have finished before modifying the 
discard_zeroes_data flag - the kind of operation that is performed by 
e.g. blk_mq_freeze_queue(). Modifying the discard_zeroes_data flag after 
a bio has been submitted and before it has completed could lead to 
several classes of subtle bugs. Functions like __blkdev_issue_discard() 
assume that the value of the discard_zeroes_data flag does not change 
after this function has been called and before the submitted requests 
completed.

Bart.

WARNING: multiple messages have this Message-ID (diff)
From: Bart Van Assche <bart.vanassche@sandisk.com>
To: Mikulas Patocka <mpatocka@redhat.com>
Cc: Jens Axboe <axboe@kernel.dk>,
	Christoph Hellwig <hch@infradead.org>,
	Mike Snitzer <snitzer@redhat.com>,
	linux-block@vger.kernel.org, dm-devel@redhat.com,
	"Alasdair G. Kergon" <agk@redhat.com>
Subject: Re: [dm-devel] [PATCH 1/4] brd: handle misaligned discard
Date: Fri, 28 Oct 2016 08:55:26 -0700	[thread overview]
Message-ID: <f3257790-19a5-8512-df37-78e5fd8a8973@sandisk.com> (raw)
In-Reply-To: <alpine.LRH.2.02.1610280731410.22676@file01.intranet.prod.int.rdu2.redhat.com>

On 10/28/2016 04:39 AM, Mikulas Patocka wrote:
> On Wed, 26 Oct 2016, Bart Van Assche wrote:
>> On 10/26/2016 02:46 PM, Mikulas Patocka wrote:
>>> I think the proper thing would be to move "discard_zeroes_data" flag into
>>> the bio itself - there would be REQ_OP_DISCARD and REQ_OP_DISCARD_ZERO -
>>> and if the device doesn't support REQ_OP_DISCARD_ZERO, it rejects the bio
>>> and the caller is supposed to do zeroing manually.
>>
>> Sorry but I don't like this proposal. I think that a much better solution
>> would be to pause I/O before making any changes to the discard_zeroes_data
>> flag.
>
> The device mapper pauses all bios when it switches the table - but the bio
> was submitted with assumption that it goes to a device withe
> "discard_zeroes_data" set, then the bio is paused, device mapper table is
> switched, the bio is unpaused, and now it can go to a device without
> "discard_zeroes_data".

Hello Mikulas,

Sorry if I wasn't clear enough. What I meant is to wait until all 
outstanding requests have finished before modifying the 
discard_zeroes_data flag - the kind of operation that is performed by 
e.g. blk_mq_freeze_queue(). Modifying the discard_zeroes_data flag after 
a bio has been submitted and before it has completed could lead to 
several classes of subtle bugs. Functions like __blkdev_issue_discard() 
assume that the value of the discard_zeroes_data flag does not change 
after this function has been called and before the submitted requests 
completed.

Bart.

  reply	other threads:[~2016-10-28 15:55 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-10-21 18:33 device mapper and the BLKFLSBUF ioctl Mikulas Patocka
2016-10-21 20:00 ` Mike Snitzer
2016-10-21 20:18   ` Mikulas Patocka
2016-10-24 15:57     ` Mike Snitzer
2016-10-25 13:07       ` Christoph Hellwig
2016-10-25 14:37         ` [PATCH] brd: remove support for BLKFLSBUF Mike Snitzer
2016-10-25 14:46           ` Jens Axboe
2016-10-26 20:25             ` [PATCH 0/4] brd: support discard Mikulas Patocka
2016-10-26 20:26               ` [PATCH 1/4] brd: handle misaligned discard Mikulas Patocka
2016-10-26 20:38                 ` [dm-devel] " Bart Van Assche
2016-10-26 20:38                   ` Bart Van Assche
2016-10-26 21:46                   ` Mikulas Patocka
2016-10-26 21:50                     ` REQ_OP for zeroing, was " Christoph Hellwig
2016-10-28 11:43                       ` Mikulas Patocka
2016-10-28 13:14                         ` Christoph Hellwig
2016-10-31 16:36                           ` Mikulas Patocka
2016-10-26 21:57                     ` Bart Van Assche
2016-10-26 21:57                       ` Bart Van Assche
2016-10-28 11:39                       ` Mikulas Patocka
2016-10-28 15:55                         ` Bart Van Assche [this message]
2016-10-28 15:55                           ` Bart Van Assche
2016-10-31 16:31                           ` Mikulas Patocka
2016-10-26 20:26               ` [PATCH 2/4] brd: extend rcu read sections Mikulas Patocka
2016-10-26 20:27               ` [PATCH 3/4] brd: implement discard Mikulas Patocka
2016-10-26 20:27               ` [PATCH 4/4] brd: remove unused brd_zero_page Mikulas Patocka

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f3257790-19a5-8512-df37-78e5fd8a8973@sandisk.com \
    --to=bart.vanassche@sandisk.com \
    --cc=agk@redhat.com \
    --cc=axboe@kernel.dk \
    --cc=dm-devel@redhat.com \
    --cc=hch@infradead.org \
    --cc=linux-block@vger.kernel.org \
    --cc=mpatocka@redhat.com \
    --cc=snitzer@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.