From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.4 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 266B8C43331 for ; Tue, 24 Mar 2020 17:18:32 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id E5DCF20774 for ; Tue, 24 Mar 2020 17:18:31 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="P4sGruV0" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727509AbgCXRSa (ORCPT ); Tue, 24 Mar 2020 13:18:30 -0400 Received: from us-smtp-delivery-74.mimecast.com ([216.205.24.74]:31701 "EHLO us-smtp-delivery-74.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727219AbgCXRSa (ORCPT ); Tue, 24 Mar 2020 13:18:30 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1585070308; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=cpYH2FJf0rR+384pL86Oqy9Nf/j9qVaPFc27qLwhHJs=; b=P4sGruV0HhCzPbM6JspBF0B4M9U55fIxLJdKzeV8h9ykirTXjy7QBbva01/03u9kXTzB9L jHwa1Iq3cnMpLOmbEaLfVUAKFh08d23bxjUQwdOL7yuo01tvnBPy7ixS19y3x9MbZ8z8wv mirTfyhFbPw0idhAWyzS1icHrWBQCGc= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-377-dRAJVmGzPaSOTfJwp7PC5g-1; Tue, 24 Mar 2020 13:18:26 -0400 X-MC-Unique: dRAJVmGzPaSOTfJwp7PC5g-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id D4D751005509; Tue, 24 Mar 2020 17:18:25 +0000 (UTC) Received: from localhost (unknown [10.18.25.174]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 001AD60BE0; Tue, 24 Mar 2020 17:18:22 +0000 (UTC) Date: Tue, 24 Mar 2020 13:18:22 -0400 From: Mike Snitzer To: Lukas Straub Cc: linux-kernel , Alasdair Kergon , dm-devel , Mikulas Patocka Subject: Re: [PATCH v2] dm-integrity: Prevent RMW for full metadata buffer writes Message-ID: <20200324171821.GA2444@redhat.com> References: <20200227142601.61f3cd54@luklap> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200227142601.61f3cd54@luklap> User-Agent: Mutt/1.5.21 (2010-09-15) X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Feb 27 2020 at 8:26am -0500, Lukas Straub wrote: > If a full metadata buffer is being written, don't read it first. This > prevents a read-modify-write cycle and increases performance on HDDs > considerably. > > To do this we now calculate the checksums for all sectors in the bio in one > go in integrity_metadata and then pass the result to dm_integrity_rw_tag, > which now checks if we overwrite the whole buffer. > > Benchmarking with a 5400RPM HDD with bitmap mode: > integritysetup format --no-wipe --batch-mode --interleave-sectors $((64*1024)) -t 4 -s 512 -I crc32c -B /dev/sdc > integritysetup open --buffer-sectors=1 -I crc32c -B /dev/sdc hdda_integ > dd if=/dev/zero of=/dev/mapper/hdda_integ bs=64K count=$((16*1024*4)) conv=fsync oflag=direct status=progress > > Without patch: > 4294967296 bytes (4.3 GB, 4.0 GiB) copied, 400.326 s, 10.7 MB/s > > With patch: > 4294967296 bytes (4.3 GB, 4.0 GiB) copied, 41.2057 s, 104 MB/s > > Signed-off-by: Lukas Straub > --- > Hello Everyone, > So here is v2, now checking if we overwrite a whole metadata buffer instead > of the (buggy) check if we overwrite a whole tag area before. > Performance stayed the same (with --buffer-sectors=1). > > The only quirk now is that it advertises a very big optimal io size in the > standard configuration (where buffer_sectors=128). Is this a Problem? > > v2: > -fix dm_integrity_rw_tag to check if we overwrite a whole metadat buffer > -fix optimal io size to check if we overwrite a whole metadata buffer > > Regards, > Lukas Straub Not sure why you didn't cc Mikulas but I just checked with him and he thinks: You're only seeing a boost because you're using 512-byte sector and 512-byte buffer. Patch helps that case but hurts in most other cases (due to small buffers). Mike > drivers/md/dm-integrity.c | 81 +++++++++++++++++++++++---------------- > 1 file changed, 47 insertions(+), 34 deletions(-) > > diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c > index b225b3e445fa..a6d3cf1406df 100644 > --- a/drivers/md/dm-integrity.c > +++ b/drivers/md/dm-integrity.c > @@ -1309,9 +1309,17 @@ static int dm_integrity_rw_tag(struct dm_integrity_c *ic, unsigned char *tag, se > if (unlikely(r)) > return r; > > - data = dm_bufio_read(ic->bufio, *metadata_block, &b); > - if (IS_ERR(data)) > - return PTR_ERR(data); > + /* Don't read metadata sectors from disk if we're going to overwrite them completely */ > + if (op == TAG_WRITE && *metadata_offset == 0 \ > + && total_size >= (1U << SECTOR_SHIFT << ic->log2_buffer_sectors)) { > + data = dm_bufio_new(ic->bufio, *metadata_block, &b); > + if (IS_ERR(data)) > + return PTR_ERR(data); > + } else { > + data = dm_bufio_read(ic->bufio, *metadata_block, &b); > + if (IS_ERR(data)) > + return PTR_ERR(data); > + } > > to_copy = min((1U << SECTOR_SHIFT << ic->log2_buffer_sectors) - *metadata_offset, total_size); > dp = data + *metadata_offset; > @@ -1514,6 +1522,8 @@ static void integrity_metadata(struct work_struct *w) > { > struct dm_integrity_io *dio = container_of(w, struct dm_integrity_io, work); > struct dm_integrity_c *ic = dio->ic; > + unsigned sectors_to_process = dio->range.n_sectors; > + sector_t sector = dio->range.logical_sector; > > int r; > > @@ -1522,16 +1532,14 @@ static void integrity_metadata(struct work_struct *w) > struct bio_vec bv; > unsigned digest_size = crypto_shash_digestsize(ic->internal_hash); > struct bio *bio = dm_bio_from_per_bio_data(dio, sizeof(struct dm_integrity_io)); > - char *checksums; > + char *checksums, *checksums_ptr; > unsigned extra_space = unlikely(digest_size > ic->tag_size) ? digest_size - ic->tag_size : 0; > char checksums_onstack[HASH_MAX_DIGESTSIZE]; > - unsigned sectors_to_process = dio->range.n_sectors; > - sector_t sector = dio->range.logical_sector; > > if (unlikely(ic->mode == 'R')) > goto skip_io; > > - checksums = kmalloc((PAGE_SIZE >> SECTOR_SHIFT >> ic->sb->log2_sectors_per_block) * ic->tag_size + extra_space, > + checksums = kmalloc((dio->range.n_sectors >> ic->sb->log2_sectors_per_block) * ic->tag_size + extra_space, > GFP_NOIO | __GFP_NORETRY | __GFP_NOWARN); > if (!checksums) { > checksums = checksums_onstack; > @@ -1542,49 +1550,45 @@ static void integrity_metadata(struct work_struct *w) > } > } > > + checksums_ptr = checksums; > __bio_for_each_segment(bv, bio, iter, dio->orig_bi_iter) { > unsigned pos; > - char *mem, *checksums_ptr; > - > -again: > + char *mem; > mem = (char *)kmap_atomic(bv.bv_page) + bv.bv_offset; > pos = 0; > - checksums_ptr = checksums; > do { > integrity_sector_checksum(ic, sector, mem + pos, checksums_ptr); > - checksums_ptr += ic->tag_size; > - sectors_to_process -= ic->sectors_per_block; > + > + if (likely(checksums != checksums_onstack)) { > + checksums_ptr += ic->tag_size; > + } else { > + r = dm_integrity_rw_tag(ic, checksums, &dio->metadata_block, &dio->metadata_offset, > + ic->tag_size, !dio->write ? TAG_CMP : TAG_WRITE); > + if (unlikely(r)) > + goto internal_hash_error; > + } > + > pos += ic->sectors_per_block << SECTOR_SHIFT; > sector += ic->sectors_per_block; > - } while (pos < bv.bv_len && sectors_to_process && checksums != checksums_onstack); > + sectors_to_process -= ic->sectors_per_block; > + } while (pos < bv.bv_len && sectors_to_process); > kunmap_atomic(mem); > > - r = dm_integrity_rw_tag(ic, checksums, &dio->metadata_block, &dio->metadata_offset, > - checksums_ptr - checksums, !dio->write ? TAG_CMP : TAG_WRITE); > - if (unlikely(r)) { > - if (r > 0) { > - DMERR_LIMIT("Checksum failed at sector 0x%llx", > - (unsigned long long)(sector - ((r + ic->tag_size - 1) / ic->tag_size))); > - r = -EILSEQ; > - atomic64_inc(&ic->number_of_mismatches); > - } > - if (likely(checksums != checksums_onstack)) > - kfree(checksums); > - goto error; > - } > - > if (!sectors_to_process) > break; > + } > > - if (unlikely(pos < bv.bv_len)) { > - bv.bv_offset += pos; > - bv.bv_len -= pos; > - goto again; > + if (likely(checksums != checksums_onstack)) { > + r = dm_integrity_rw_tag(ic, checksums, &dio->metadata_block, &dio->metadata_offset, > + (dio->range.n_sectors >> ic->sb->log2_sectors_per_block) * ic->tag_size, > + !dio->write ? TAG_CMP : TAG_WRITE); > + if (unlikely(r)) { > + kfree(checksums); > + goto internal_hash_error; > } > + kfree(checksums); > } > > - if (likely(checksums != checksums_onstack)) > - kfree(checksums); > } else { > struct bio_integrity_payload *bip = dio->orig_bi_integrity; > > @@ -1615,6 +1619,13 @@ static void integrity_metadata(struct work_struct *w) > skip_io: > dec_in_flight(dio); > return; > +internal_hash_error: > + if (r > 0) { > + DMERR_LIMIT("Checksum failed at sector 0x%llx", > + (unsigned long long)(sector - ((r + ic->tag_size - 1) / ic->tag_size))); > + r = -EILSEQ; > + atomic64_inc(&ic->number_of_mismatches); > + } > error: > dio->bi_status = errno_to_blk_status(r); > dec_in_flight(dio); > @@ -3019,6 +3030,8 @@ static void dm_integrity_io_hints(struct dm_target *ti, struct queue_limits *lim > limits->physical_block_size = ic->sectors_per_block << SECTOR_SHIFT; > blk_limits_io_min(limits, ic->sectors_per_block << SECTOR_SHIFT); > } > + > + blk_limits_io_opt(limits, 1U << SECTOR_SHIFT << ic->log2_buffer_sectors >> ic->log2_tag_size << SECTOR_SHIFT ); > } > > static void calculate_journal_section_size(struct dm_integrity_c *ic) > -- > 2.20.1 >