On Wed, Feb 12, 2020 at 02:27:09PM -0800, Salman Qazi wrote:
> Hi,
>
> So, here's another issue that we are grappling with, where we have a
> root-cause but don't currently have a good fix for. BLKSECDISCARD is
> an operation used for securely destroying a subset of the data on a
> device. Unfortunately, on SSDs, this is an operation with variable
> performance. It can be O(minutes) in the worst case. The
> pathological case is when many erase blocks on the flash contain a
> small amount of data that is part of the discard and a large amount of
> data that isn't. In such cases, the erase blocks have to be copied
> almost in entirety to fresh blocks, in order to erase the sectors to
> be discarded. This can be thought of as a defragmentation operation on
> the drive and can be expected to cost in the same ballpark as
> rewriting most of the contents of the drive.
>
> Therefore, it is possible for the thread waiting in the IOCTL in
> submit_bio_wait call in blkdev_issue_discard to wait for several
> minutes. The hung task watchdog is usually configured for 2 minutes,
> and this can expire before the operation finishes.
>
> This operation is very important to the security model of Chrome OS
> devices. Therefore, we would like the kernel to survive this even if
> it takes several minutes.
>
> Three approaches come to mind:
>
> One approach is to somehow avoid waiting for a single monolithic
> operation and instead wait on bits and pieces of the operation. These
> can be sized to finish within a reasonable timeframe. The exact size
> is likely device-specific. We already split these operations before
> issuing to the device, but the IOCTL thread is waiting for the whole
> rather than the parts. The hung task watchdog only sees the total
> amount of time the thread slept and not the forward progress taking
> place quietly.
>
> Another approach might be to do something in the spirit of the write
> system call: complete the partial operation (whatever the kernel
> thinks is reasonable), adjust the IOCTL argument and have the
> userspace reissue the syscall to continue the operation. The second
> option should probably be done with a different IOCTL name to avoid
> breaking userspace.
>
> A third approach, which is perhaps more adventurous, is to have a
> notion of forward progress that a thread can export and the hung task
> watchdog can evaluate. This can take the form of a function pointer
> and an argument. The result of the function is a monotonically
> decreasing unsigned value. When this value stops changing, we can
> conclude that the thread is hung. This can be used in place of
> context switch count for tasks where this function is available. This
> can potentially solve other similar issues, there is a way to tell if
> there is forward progress, but it is not as straightforward as the
> context switch count.
>
> What are your thoughts?
The approach used in blk_execute_rq() can be borrowed for workaround the
issue, such as:
diff --git a/block/bio.c b/block/bio.c
index 94d697217887..c9ce19a86de7 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -17,6 +17,7 @@
#include <linux/cgroup.h>
#include <linux/blk-cgroup.h>
#include <linux/highmem.h>
+#include <linux/sched/sysctl.h>
#include <trace/events/block.h>
#include "blk.h"
@@ -1019,12 +1020,19 @@ static void submit_bio_wait_endio(struct bio *bio)
int submit_bio_wait(struct bio *bio)
{
DECLARE_COMPLETION_ONSTACK_MAP(done, bio->bi_disk->lockdep_map);
+ unsigned long hang_check;
bio->bi_private = &done;
bio->bi_end_io = submit_bio_wait_endio;
bio->bi_opf |= REQ_SYNC;
submit_bio(bio);
- wait_for_completion_io(&done);
+
+ /* Prevent hang_check timer from firing at us during very long I/O */
+ hang_check = sysctl_hung_task_timeout_secs;
+ if (hang_check)
+ while (!wait_for_completion_io_timeout(&done, hang_check * (HZ/2)));
+ else
+ wait_for_completion_io(&done);
return blk_status_to_errno(bio->bi_status);
}
thanks,
Ming
