Re: [dm-devel] [PATCH] Improve the dm-integrity documentation.

Re: [dm-devel] [PATCH] Improve the dm-integrity documentation.

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 5664 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH] Improve the dm-integrity documentation.

[Russell Harmon]

Thanks for taking a look. Here's the multi-patch series plus the changes
you suggested.

[PATCH 1/4] Fix minor grammatical error in dm-integrity.rst.

[Russell Harmon]

Signed-off-by: Russell Harmon <eatnumber1@gmail.com>
---
 Documentation/admin-guide/device-mapper/dm-integrity.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/device-mapper/dm-integrity.rst b/Documentation/admin-guide/device-mapper/dm-integrity.rst
index 8db172efa272..b2a698e955a3 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
 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
-- 
2.34.1

[PATCH 2/4] Documents the meaning of "buffer" in dm-integrity.

[Russell Harmon]

Signed-off-by: Russell Harmon <eatnumber1@gmail.com>
---
 .../admin-guide/device-mapper/dm-integrity.rst      | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/device-mapper/dm-integrity.rst b/Documentation/admin-guide/device-mapper/dm-integrity.rst
index b2a698e955a3..31f514675809 100644
--- a/Documentation/admin-guide/device-mapper/dm-integrity.rst
+++ b/Documentation/admin-guide/device-mapper/dm-integrity.rst
@@ -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) 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. 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
@@ -106,10 +115,6 @@ buffer_sectors:number
 	The number of sectors in one 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
 	The journal watermark in percents. When the size of the journal
 	exceeds this watermark, the thread that flushes the journal will
-- 
2.34.1

[PATCH 3/4] Document dm-integrity default values.

[Russell Harmon]

Signed-off-by: Russell Harmon <eatnumber1@gmail.com>
---
 .../device-mapper/dm-integrity.rst            | 22 +++++++++----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/Documentation/admin-guide/device-mapper/dm-integrity.rst b/Documentation/admin-guide/device-mapper/dm-integrity.rst
index 31f514675809..0241457c0027 100644
--- a/Documentation/admin-guide/device-mapper/dm-integrity.rst
+++ b/Documentation/admin-guide/device-mapper/dm-integrity.rst
@@ -102,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.
@@ -111,16 +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.
 
-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.
@@ -168,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
@@ -291,7 +290,8 @@ The layout of the formatted block device:
     Each run contains:
 
 	* tag area - it contains integrity tags. There is one tag for each
-	  sector in the data area
+	  sector in the data area. The size of this area is always 4KiB or
+	  greater.
 	* data area - it contains data sectors. The number of data sectors
 	  in one run must be a power of two. log2 of this value is stored
 	  in the superblock.
-- 
2.34.1

[PATCH 4/4] Document an example of how the tunables relate in dm-integrity.

[Russell Harmon]

Signed-off-by: Russell Harmon <eatnumber1@gmail.com>
---
 Documentation/admin-guide/device-mapper/dm-integrity.rst | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/admin-guide/device-mapper/dm-integrity.rst b/Documentation/admin-guide/device-mapper/dm-integrity.rst
index 0241457c0027..d8a5f14d0e3c 100644
--- a/Documentation/admin-guide/device-mapper/dm-integrity.rst
+++ b/Documentation/admin-guide/device-mapper/dm-integrity.rst
@@ -213,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.
 
 Status line:
 
-- 
2.34.1

Re: [PATCH] Improve the dm-integrity documentation.

[Bagas Sanjaya]

On 6/4/23 03:15, Russell Harmon wrote:
> Thanks for taking a look. Here's the multi-patch series plus the changes
> you suggested.
> 
> 

This is v2, right? Next reroll, remember to version your patches
(pass -v to git-format-patch(1)).

Also, I don't see description (that would be commit message
once applied by jon) in all patches on this series. I hope you
have a time reading Documentation/process/submitting-patches.rst
so that trivial mistakes like this shouldn't happen again. In
any case, when preparing patch series, always add cover letter
by passing --cover-letter to git-format-patch(1) then edit the
generated template (look for 0000-*-cover-letter.patch).

Thanks.

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

Re: [PATCH] Improve the dm-integrity documentation.

[Russell Harmon]

On Sun, Jun 4, 2023 at 7:07 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
>
> On 6/4/23 03:15, Russell Harmon wrote:
> > Thanks for taking a look. Here's the multi-patch series plus the changes
> > you suggested.
> >
> >
>
> This is v2, right? Next reroll, remember to version your patches
> (pass -v to git-format-patch(1)).

Will do.

> Also, I don't see description (that would be commit message
> once applied by jon) in all patches on this series. I hope you
> have a time reading Documentation/process/submitting-patches.rst
> so that trivial mistakes like this shouldn't happen again. In
> any case, when preparing patch series, always add cover letter
> by passing --cover-letter to git-format-patch(1) then edit the
> generated template (look for 0000-*-cover-letter.patch).

My "thanks for taking a look" email was the cover letter.

Interesting that the commit message didn't go through in the body,
they're each one line and were moved to the subject line. I'll try
again and add a little more description.

[PATCH v3 0/4] Improve the dm-integrity documentation.

[Russell Harmon]

Version 3, thanks Bagas for working with me.

Russell Harmon (4):
  Fix minor grammatical error in dm-integrity.rst.
  Documents the meaning of "buffer" in dm-integrity.
  Document dm-integrity default values.
  Document an example of how the tunables relate in dm-integrity.

 .../device-mapper/dm-integrity.rst            | 43 ++++++++++++-------
 1 file changed, 27 insertions(+), 16 deletions(-)

-- 
2.34.1

[PATCH v3 1/4] Fix minor grammatical error in dm-integrity.rst.

[Russell Harmon]

"where dm-integrity uses bitmap" becomes "where dm-integrity uses a
bitmap"

Signed-off-by: Russell Harmon <eatnumber1@gmail.com>
---
 Documentation/admin-guide/device-mapper/dm-integrity.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/device-mapper/dm-integrity.rst b/Documentation/admin-guide/device-mapper/dm-integrity.rst
index 8db172efa272..b2a698e955a3 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
 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
-- 
2.34.1

Re: [PATCH v3 1/4] Fix minor grammatical error in dm-integrity.rst.

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 282 bytes --]

On Sun, Jun 04, 2023 at 12:06:01PM -0700, Russell Harmon wrote:
> "where dm-integrity uses bitmap" becomes "where dm-integrity uses a
> bitmap"

Do you refer to one bitmap or uncountable bitmaps?

I'm confused...

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 1/4] Fix minor grammatical error in dm-integrity.rst.

[Russell Harmon]

On Sun, Jun 4, 2023 at 8:03 PM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
>
> On Sun, Jun 04, 2023 at 12:06:01PM -0700, Russell Harmon wrote:
> > "where dm-integrity uses bitmap" becomes "where dm-integrity uses a
> > bitmap"
>
> Do you refer to one bitmap or uncountable bitmaps?
>
> I'm confused...

There is only one bitmap on-disk, written to by bitmap_flush_work [1].
In-memory there's many (it's stored as a linked list of pages), but
the docs seem to be implicitly referring to the on-disk
representation, so this would therefore be referring to a singular map
of bits, or "a bitmap."

[1]: https://github.com/torvalds/linux/blob/master/drivers/md/dm-integrity.c#L2876-L2877

[PATCH v3 2/4] Documents the meaning of "buffer" in dm-integrity.

[Russell Harmon]

"Buffers" are buffers of the metadata/checksum area of dm-integrity.
They are always at most as large as a single metadata area on-disk, but
may be smaller.

Signed-off-by: Russell Harmon <eatnumber1@gmail.com>
---
 .../admin-guide/device-mapper/dm-integrity.rst      | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/device-mapper/dm-integrity.rst b/Documentation/admin-guide/device-mapper/dm-integrity.rst
index b2a698e955a3..31f514675809 100644
--- a/Documentation/admin-guide/device-mapper/dm-integrity.rst
+++ b/Documentation/admin-guide/device-mapper/dm-integrity.rst
@@ -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) 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. 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
@@ -106,10 +115,6 @@ buffer_sectors:number
 	The number of sectors in one 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
 	The journal watermark in percents. When the size of the journal
 	exceeds this watermark, the thread that flushes the journal will
-- 
2.34.1

Re: [PATCH v3 2/4] Documents the meaning of "buffer" in dm-integrity.

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 794 bytes --]

On Sun, Jun 04, 2023 at 12:06:02PM -0700, Russell Harmon wrote:
> +Accesses to the on-disk metadata area containing checksums (aka tags) 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. The metadata is still read even in
> +a full write to the data covered by a single buffer.
> +

LGTM, thanks!

Reviewed-by: Bagas Sanjaya <bagasdotme@gmail.com>

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 2/4] Documents the meaning of "buffer" in dm-integrity.

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 454 bytes --]

On Sun, Jun 04, 2023 at 12:06:02PM -0700, Russell Harmon wrote:
> "Buffers" are buffers of the metadata/checksum area of dm-integrity.
> They are always at most as large as a single metadata area on-disk, but
> may be smaller.

Hey, I forgot to mention below.

Where is the subject prefix? The patch title should have been "Documentation: dm-integrity: Document
buffers".

Thanks.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 2/4] Documents the meaning of "buffer" in dm-integrity.

[Russell Harmon]

On Sun, Jun 4, 2023 at 8:08 PM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
>
> On Sun, Jun 04, 2023 at 12:06:02PM -0700, Russell Harmon wrote:
> > "Buffers" are buffers of the metadata/checksum area of dm-integrity.
> > They are always at most as large as a single metadata area on-disk, but
> > may be smaller.
>
> Hey, I forgot to mention below.
>
> Where is the subject prefix? The patch title should have been "Documentation: dm-integrity: Document
> buffers".

Ah I missed that in the docs. I'll send a v4 with it included.

[PATCH v3 3/4] Document dm-integrity default values.

[Russell Harmon]

Specifically:
  interleave_sectors = 32768
  buffer_sectors = 128
  block_size = 512
  journal_watermark = 50
  commit_time = 10000

Signed-off-by: Russell Harmon <eatnumber1@gmail.com>
---
 .../device-mapper/dm-integrity.rst            | 22 +++++++++----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/Documentation/admin-guide/device-mapper/dm-integrity.rst b/Documentation/admin-guide/device-mapper/dm-integrity.rst
index 31f514675809..0241457c0027 100644
--- a/Documentation/admin-guide/device-mapper/dm-integrity.rst
+++ b/Documentation/admin-guide/device-mapper/dm-integrity.rst
@@ -102,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.
@@ -111,16 +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.
 
-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.
@@ -168,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
@@ -291,7 +290,8 @@ The layout of the formatted block device:
     Each run contains:
 
 	* tag area - it contains integrity tags. There is one tag for each
-	  sector in the data area
+	  sector in the data area. The size of this area is always 4KiB or
+	  greater.
 	* data area - it contains data sectors. The number of data sectors
 	  in one run must be a power of two. log2 of this value is stored
 	  in the superblock.
-- 
2.34.1

Re: [PATCH v3 3/4] Document dm-integrity default values.

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 454 bytes --]

On Sun, Jun 04, 2023 at 12:06:03PM -0700, Russell Harmon wrote:
> Specifically:
>   interleave_sectors = 32768
>   buffer_sectors = 128
>   block_size = 512
>   journal_watermark = 50
>   commit_time = 10000

Your patch description duplicates the diff content below. Please write
in a mood that evocates curiosity to read the diff (and make sure it is
also imperative).

Thanks.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 3/4] Document dm-integrity default values.

[Russell Harmon]

On Sun, Jun 4, 2023 at 8:16 PM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
>
> On Sun, Jun 04, 2023 at 12:06:03PM -0700, Russell Harmon wrote:
> > Specifically:
> >   interleave_sectors = 32768
> >   buffer_sectors = 128
> >   block_size = 512
> >   journal_watermark = 50
> >   commit_time = 10000
>
> Your patch description duplicates the diff content below. Please write
> in a mood that evocates curiosity to read the diff (and make sure it is
> also imperative).

I'm not sure what to say in the commit message that isn't already
captured in the diff content and the summary line of the commit. v2
was such a patch, but you asked to add some content to the message. Do
you have a suggestion for what to say?

v4 that I'm about to send says "Documentation: dm-integrity: Document
default values" in the summary line and has no other content in the
commit message.

Re: [PATCH v3 3/4] Document dm-integrity default values.

[Jonathan Corbet]

Bagas Sanjaya <bagasdotme@gmail.com> writes:

> On Sun, Jun 04, 2023 at 12:06:03PM -0700, Russell Harmon wrote:
>> Specifically:
>>   interleave_sectors = 32768
>>   buffer_sectors = 128
>>   block_size = 512
>>   journal_watermark = 50
>>   commit_time = 10000
>
> Your patch description duplicates the diff content below. Please write
> in a mood that evocates curiosity to read the diff (and make sure it is
> also imperative).

Bagas, this is a typo-fix patch.  It does not need to be nitpicked into
the ground.

Russell, Bagas is inadvertently teaching you another lesson about the
kernel development community; it includes a certain number of people who
like to push contributors around with authoritative-sounding messages.
These people do not always need to be listened to.

Thanks,

jon

Re: [PATCH v3 3/4] Document dm-integrity default values.

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]

On Mon, Jun 05, 2023 at 07:23:34AM -0600, Jonathan Corbet wrote:
> Bagas Sanjaya <bagasdotme@gmail.com> writes:
> 
> > On Sun, Jun 04, 2023 at 12:06:03PM -0700, Russell Harmon wrote:
> >> Specifically:
> >>   interleave_sectors = 32768
> >>   buffer_sectors = 128
> >>   block_size = 512
> >>   journal_watermark = 50
> >>   commit_time = 10000
> >
> > Your patch description duplicates the diff content below. Please write
> > in a mood that evocates curiosity to read the diff (and make sure it is
> > also imperative).
> 
> Bagas, this is a typo-fix patch.  It does not need to be nitpicked into
> the ground.
> 
> Russell, Bagas is inadvertently teaching you another lesson about the
> kernel development community; it includes a certain number of people who
> like to push contributors around with authoritative-sounding messages.
> These people do not always need to be listened to.

OK, thanks for the tip again!

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v3 4/4] Document an example of how the tunables relate in dm-integrity.

[Russell Harmon]

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.

Signed-off-by: Russell Harmon <eatnumber1@gmail.com>
---
 Documentation/admin-guide/device-mapper/dm-integrity.rst | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/admin-guide/device-mapper/dm-integrity.rst b/Documentation/admin-guide/device-mapper/dm-integrity.rst
index 0241457c0027..d8a5f14d0e3c 100644
--- a/Documentation/admin-guide/device-mapper/dm-integrity.rst
+++ b/Documentation/admin-guide/device-mapper/dm-integrity.rst
@@ -213,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.
 
 Status line:
 
-- 
2.34.1

Re: [PATCH v3 4/4] Document an example of how the tunables relate in dm-integrity.

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

On Sun, Jun 04, 2023 at 12:06:04PM -0700, Russell Harmon wrote:
> 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.

Again, the patch description duplicates diff content.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 4/4] Document an example of how the tunables relate in dm-integrity.

[Russell Harmon]

Same as with the patch adding default values. Can you recommend
something to say in the commit beyond the summary line?

On Sun, Jun 4, 2023 at 8:17 PM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
>
> On Sun, Jun 04, 2023 at 12:06:04PM -0700, Russell Harmon wrote:
> > 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.
>
> Again, the patch description duplicates diff content.
>
> --
> An old man doll... just what I always wanted! - Clara

[PATCH v4 0/4] Improve the dm-integrity documentation.

[Russell Harmon]

Russell Harmon (4):
  Documentation: dm-integrity: Fix minor grammatical error.
  Documentation: dm-integrity: Document the meaning of "buffer".
  Documentation: dm-integrity: Document default values.
  Documentation: dm-integrity: Document an example of how the tunables
    relate.

 .../device-mapper/dm-integrity.rst            | 43 ++++++++++++-------
 1 file changed, 27 insertions(+), 16 deletions(-)

-- 
2.34.1

[PATCH v4 1/4] Documentation: dm-integrity: Fix minor grammatical error.

[Russell Harmon]

"where dm-integrity uses bitmap" becomes "where dm-integrity uses a
bitmap"

Signed-off-by: Russell Harmon <eatnumber1@gmail.com>
---
 Documentation/admin-guide/device-mapper/dm-integrity.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/device-mapper/dm-integrity.rst b/Documentation/admin-guide/device-mapper/dm-integrity.rst
index 8db172efa272..b2a698e955a3 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
 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
-- 
2.34.1

Re: [PATCH v4 1/4] Documentation: dm-integrity: Fix minor grammatical error.

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 347 bytes --]

On Sun, Jun 04, 2023 at 10:08:50PM -0700, Russell Harmon wrote:
> -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

LGTM, thanks!

Reviewed-by: Bagas Sanjaya <bagasdotme@gmail.com>

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v4 2/4] Documentation: dm-integrity: Document the meaning of "buffer".

[Russell Harmon]

"Buffers" are buffers of the metadata/checksum area of dm-integrity.
They are always at most as large as a single metadata area on-disk, but
may be smaller.

Signed-off-by: Russell Harmon <eatnumber1@gmail.com>
---
 .../admin-guide/device-mapper/dm-integrity.rst      | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/device-mapper/dm-integrity.rst b/Documentation/admin-guide/device-mapper/dm-integrity.rst
index b2a698e955a3..31f514675809 100644
--- a/Documentation/admin-guide/device-mapper/dm-integrity.rst
+++ b/Documentation/admin-guide/device-mapper/dm-integrity.rst
@@ -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) 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. 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
@@ -106,10 +115,6 @@ buffer_sectors:number
 	The number of sectors in one 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
 	The journal watermark in percents. When the size of the journal
 	exceeds this watermark, the thread that flushes the journal will
-- 
2.34.1

Re: [PATCH v4 2/4] Documentation: dm-integrity: Document the meaning of "buffer".

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 794 bytes --]

On Sun, Jun 04, 2023 at 10:08:51PM -0700, Russell Harmon wrote:
> +Accesses to the on-disk metadata area containing checksums (aka tags) 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. The metadata is still read even in
> +a full write to the data covered by a single buffer.
> +

LGTM, thanks!

Reviewed-by: Bagas Sanjaya <bagasdotme@gmail.com>

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v4 3/4] Documentation: dm-integrity: Document default values.

[Russell Harmon]

Signed-off-by: Russell Harmon <eatnumber1@gmail.com>
---
 .../device-mapper/dm-integrity.rst            | 22 +++++++++----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/Documentation/admin-guide/device-mapper/dm-integrity.rst b/Documentation/admin-guide/device-mapper/dm-integrity.rst
index 31f514675809..0241457c0027 100644
--- a/Documentation/admin-guide/device-mapper/dm-integrity.rst
+++ b/Documentation/admin-guide/device-mapper/dm-integrity.rst
@@ -102,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.
@@ -111,16 +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.
 
-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.
@@ -168,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
@@ -291,7 +290,8 @@ The layout of the formatted block device:
     Each run contains:
 
 	* tag area - it contains integrity tags. There is one tag for each
-	  sector in the data area
+	  sector in the data area. The size of this area is always 4KiB or
+	  greater.
 	* data area - it contains data sectors. The number of data sectors
 	  in one run must be a power of two. log2 of this value is stored
 	  in the superblock.
-- 
2.34.1

[PATCH v4 4/4] Documentation: dm-integrity: Document an example of how the tunables relate.

[Russell Harmon]

Signed-off-by: Russell Harmon <eatnumber1@gmail.com>
---
 Documentation/admin-guide/device-mapper/dm-integrity.rst | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/admin-guide/device-mapper/dm-integrity.rst b/Documentation/admin-guide/device-mapper/dm-integrity.rst
index 0241457c0027..d8a5f14d0e3c 100644
--- a/Documentation/admin-guide/device-mapper/dm-integrity.rst
+++ b/Documentation/admin-guide/device-mapper/dm-integrity.rst
@@ -213,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.
 
 Status line:
 
-- 
2.34.1

Re: [PATCH v4 4/4] Documentation: dm-integrity: Document an example of how the tunables relate.

[Russell Harmon]

Friendly ping on this last patch. Was there additional changes needed
for this one?

On Sun, Jun 4, 2023 at 10:09 PM Russell Harmon <eatnumber1@gmail.com> wrote:
>
> Signed-off-by: Russell Harmon <eatnumber1@gmail.com>
> ---
>  Documentation/admin-guide/device-mapper/dm-integrity.rst | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/admin-guide/device-mapper/dm-integrity.rst b/Documentation/admin-guide/device-mapper/dm-integrity.rst
> index 0241457c0027..d8a5f14d0e3c 100644
> --- a/Documentation/admin-guide/device-mapper/dm-integrity.rst
> +++ b/Documentation/admin-guide/device-mapper/dm-integrity.rst
> @@ -213,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.
>
>  Status line:
>
> --
> 2.34.1
>

Re: [dm-devel] [PATCH] Improve the dm-integrity documentation.

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 753 bytes --]

On Sat, Jun 03, 2023 at 03:02:09AM -0700, Russell Harmon wrote:
> Apologies for my inexperience here, but is this patch likely to get
> included? I see there's been no discussion.
> 

tl;dr:

> A: http://en.wikipedia.org/wiki/Top_post
> Q: Were do I find info about this thing called top-posting?
> A: Because it messes up the order in which people normally read text.
> Q: Why is top-posting such a bad thing?
> A: Top-posting.
> Q: What is the most annoying thing in e-mail?
> 
> A: No.
> Q: Should I include quotations after my reply?
> 
> http://daringfireball.net/2007/07/on_top

Next time don't top-post here; reply inline with appropriate context instead.

Bye!

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]