On Mon, May 29, 2023 at 05:20:32PM -0700, Russell Harmon wrote:
> Documents the meaning of the "buffer", adds documentation of the current
> defaults, and provides an example of how the tunables relate to each
> other.

What about splitting this patch into three-patch series, where each
patch do one improv?

Anyway, I'm reviewing the wording here.

> diff --git a/Documentation/admin-guide/device-mapper/dm-integrity.rst b/Documentation/admin-guide/device-mapper/dm-integrity.rst
> index 8db172efa272..634a780d7c51 100644
> --- a/Documentation/admin-guide/device-mapper/dm-integrity.rst
> +++ b/Documentation/admin-guide/device-mapper/dm-integrity.rst
> @@ -25,7 +25,7 @@ mode it calculates and verifies the integrity tag internally. In this
>  mode, the dm-integrity target can be used to detect silent data
>  corruption on the disk or in the I/O path.
>  
> -There's an alternate mode of operation where dm-integrity uses bitmap
> +There's an alternate mode of operation where dm-integrity uses a bitmap

OK.

>  instead of a journal. If a bit in the bitmap is 1, the corresponding
>  region's data and integrity tags are not synchronized - if the machine
>  crashes, the unsynchronized regions will be recalculated. The bitmap mode
> @@ -38,6 +38,15 @@ the device. But it will only format the device if the superblock contains
>  zeroes. If the superblock is neither valid nor zeroed, the dm-integrity
>  target can't be loaded.
>  
> +Accesses to the on-disk metadata area containing checksums (aka tags) is
"Accesses to ... are ..."
> +buffered using dm-bufio. When an access to any given metadata area
> +occurs, each unique metadata area gets its own buffer(s). The buffer size
> +is capped at the size of the metadata area, but may be smaller, thereby
> +requiring multiple buffers to represent the full metadata area. A smaller
> +buffer size will produce a smaller resulting read/write operation to the
> +metadata area for small reads/writes. A full write to the data covered by
> +a single buffer does *not* skip the read of the metadata.
What about "The metadata is still read even in a full write to the data
covered by a single buffer."?

> +
>  To use the target for the first time:
>  
>  1. overwrite the superblock with zeroes
> @@ -93,7 +102,7 @@ journal_sectors:number
>  	device. If the device is already formatted, the value from the
>  	superblock is used.
>  
> -interleave_sectors:number
> +interleave_sectors:number (default 32768)
>  	The number of interleaved sectors. This values is rounded down to
>  	a power of two. If the device is already formatted, the value from
>  	the superblock is used.
> @@ -102,20 +111,16 @@ meta_device:device
>  	Don't interleave the data and metadata on the device. Use a
>  	separate device for metadata.
>  
> -buffer_sectors:number
> -	The number of sectors in one buffer. The value is rounded down to
> -	a power of two.
> +buffer_sectors:number (default 128)
> +	The number of sectors in one metadata buffer. The value is rounded
> +	down to a power of two.
>  
> -	The tag area is accessed using buffers, the buffer size is
> -	configurable. The large buffer size means that the I/O size will
> -	be larger, but there could be less I/Os issued.
> -
> -journal_watermark:number
> +journal_watermark:number (default 50)
>  	The journal watermark in percents. When the size of the journal
>  	exceeds this watermark, the thread that flushes the journal will
>  	be started.
>  
> -commit_time:number
> +commit_time:number (default 10000)
>  	Commit time in milliseconds. When this time passes, the journal is
>  	written. The journal is also written immediately if the FLUSH
>  	request is received.
> @@ -163,11 +168,10 @@ journal_mac:algorithm(:key)	(the key is optional)
>  	the journal. Thus, modified sector number would be detected at
>  	this stage.
>  
> -block_size:number
> -	The size of a data block in bytes.  The larger the block size the
> +block_size:number (default 512)
> +	The size of a data block in bytes. The larger the block size the
>  	less overhead there is for per-block integrity metadata.
> -	Supported values are 512, 1024, 2048 and 4096 bytes.  If not
> -	specified the default block size is 512 bytes.
> +	Supported values are 512, 1024, 2048 and 4096 bytes.
>  
>  sectors_per_bit:number
>  	In the bitmap mode, this parameter specifies the number of
> @@ -209,6 +213,12 @@ table and swap the tables with suspend and resume). The other arguments
>  should not be changed when reloading the target because the layout of disk
>  data depend on them and the reloaded target would be non-functional.
>  
> +For example, on a device using the default interleave_sectors of 32768, a
> +block_size of 512, and an internal_hash of crc32c with a tag size of 4
> +bytes, it will take 128 KiB of tags to track a full data area, requiring
> +256 sectors of metadata per data area. With the default buffer_sectors of
> +128, that means there will be 2 buffers per metadata area, or 2 buffers
> +per 16 MiB of data.

OK.

>  
>  Status line:
>  
> @@ -285,8 +295,8 @@ The layout of the formatted block device:
>  * one or more runs of interleaved tags and data.
>      Each run contains:
>  
> -	* tag area - it contains integrity tags. There is one tag for each
> -	  sector in the data area
> +	* tag area - it contains integrity tags. There is one tag for each sector in
> +	  the data area. The size of this area is always 4KiB or greater.
Corresponding sector in the data area?

Thanks.

-- 
An old man doll... just what I always wanted! - Clara