Message-ID: <87746167-2352-c4f5-bc63-38cb3e14759f@gmail.com>
Date:   Wed, 12 Oct 2022 23:50:53 +0200
From:   Alejandro Colomar <alx.manpages@...il.com>
To:     Eric Biggers <ebiggers@...nel.org>,
        Christian Brauner <brauner@...nel.org>,
        linux-man@...r.kernel.org
Cc:     linux-fsdevel@...r.kernel.org, linux-api@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-ext4@...r.kernel.org,
        linux-f2fs-devel@...ts.sourceforge.net, linux-xfs@...r.kernel.org,
        linux-fscrypt@...r.kernel.org, linux-block@...r.kernel.org,
        "Darrick J. Wong" <djwong@...nel.org>,
        Michael Kerrisk <mtk.manpages@...il.com>
Subject: Re: [man-pages PATCH v4] statx.2, open.2: document STATX_DIOALIGN

Hi Eric,

On 10/12/22 00:59, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@...gle.com>
> 
> Document the STATX_DIOALIGN support for statx()
> (https://git.kernel.org/linus/725737e7c21d2d25).
> 
> Reviewed-by: Darrick J. Wong <djwong@...nel.org>
> Signed-off-by: Eric Biggers <ebiggers@...gle.com>

Patch applied.  Plus a pair of minor tweaks (see below 2 comments inline 
if you're curious).

Thanks,

Alex

P.S.:
The line breaks were fine.  I appreciate it very much.  Of course, if 
there had been minor issues with it, I could fix them in an amend. 
However, I prefer if I don't have to reformat entirely all of the 
patches that I review.  I hope you understand that if I were supposed to 
do that, I'd resign soon, or someone would have to pay me for doing it. 
It's a boring task.

That's why I try to teach contributors, and especially kernel 
maintainers --which send me the biggest patches I have to review-- how 
to format text nicely from the beginning.  I understand that the first 
few times it's difficult to see the benefit compared to the cost.  I can 
tell you that it affects significantly the amount of work I need to do.

Michael had a much less strict policy in this regard.  He basically 
reformatted a lot of stuff afterwards in a separate commit.  I don't 
like that approach for several reasons:
- If I don't tell you that there's a problem, you don't even know 
there's a problem.  The problem therefore will persist.
- The amount of work required for that is insane (just search the git 
log; there are hundreds of commits from Michael being applied right 
after a patch fixing the formatting).  He might have been willing to do 
it; I'm not.
- There are some fixes that might not be worth fixing after applying a 
commit, sometimes for fear of churn, sometimes for laziness.  Both 
avoiding churn and laziness are good reasons to avoid doing something, 
don't get me wrong.  But the result is that the corpus of the manual 
pages is very then inconsistent.  I prefer being a bit^W^W quite more 
picky, so that the pages have a very consistent format, which will 
trigger better patches just by following existing style.

I acknowledge the practice is a bit arbitrary, in that there are several 
equally-valid break points, which makes it a difficult task (it's easier 
when you don't need to decide).  However, it helps me significantly to 
read the patches that way (and not only small patches that modify some 
text, but also the patch that adds the whole text --by being organized 
with some logic, it's easier for me to follow it--).  Word diffs might 
be good for (very) small patches, but not for (relatively) big ones. 
However, using semantic newlines is even better than word diffs, IMO.

BTW, I don't find it more arbitrary than being able to use 'goto' vs 
'else', or other similarly controverted decisions in programming. 
Neither 'goto' or 'else' is _always_ the way to go for handling unusual 
or error cases; it's up to the good taste of the programmer to know 
which to use; sometimes either is fine really.  The same happens with 
line breaks; sometimes one point is undoubtedly better than others to 
break a line; sometimes, there are several equally-good points, and you 
can choose any.

And another BTW: this goes back to Kernighan:
<https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit?id=6ff6f43d68164f99a8c3fb66f4525d145571310c>.
Maybe he can convince you better than I do that they're a great thing.

Christian, I added you in this mail because we had this issue a long 
time ago and I didn't really answer you back then, IIRC.  Please 
consider this rationale.

Paraphrasing the Linux kernel coding style:

This is the preferred style for the linux man-pages. Style is very 
personal, and I won’t force my views on anybody, but this is what goes 
for anything that I have to be able to maintain, and I’d prefer it for 
most other things too. Please at least consider the points made here.

P.S. 2:

Please add me to 'To:' (or Cc: at least) for man-pages patches, since I 
may miss it otherwise (depending on traffic to the mailing lists that 
I'm subscribed).

Cheers,

Alex


> ---
> 
> v4: formatting tweaks, as suggested by Alejandro
> 
> v3: updated mentions of Linux version, fixed some punctuation, and added
>      a Reviewed-by
> 
> v2: rebased onto man-pages master branch, mentioned xfs, and updated
>      link to patchset
> 
>   man2/open.2  | 52 +++++++++++++++++++++++++++++++++++++++++-----------
>   man2/statx.2 | 31 +++++++++++++++++++++++++++++++
>   2 files changed, 72 insertions(+), 11 deletions(-)
> 
> diff --git a/man2/open.2 b/man2/open.2
> index 57beb324a..8e4a063b4 100644
> --- a/man2/open.2
> +++ b/man2/open.2
> @@ -1732,21 +1732,51 @@ of user-space buffers and the file offset of I/Os.
>   In Linux alignment
>   restrictions vary by filesystem and kernel version and might be
>   absent entirely.
> -However there is currently no filesystem\-independent
> -interface for an application to discover these restrictions for a given
> -file or filesystem.
> -Some filesystems provide their own interfaces
> -for doing so, for example the
> +The handling of misaligned
> +.B O_DIRECT
> +I/Os also varies;
> +they can either fail with
> +.B EINVAL
> +or fall back to buffered I/O.
> +.PP
> +Since Linux 6.1,
> +.B O_DIRECT
> +support and alignment restrictions for a file can be queried using
> +.BR statx (2),
> +using the
> +.B STATX_DIOALIGN
> +flag.
> +Support for
> +.B STATX_DIOALIGN
> +varies by filesystem;
> +see
> +.BR statx (2).
> +.PP
> +Some filesystems provide their own interfaces for querying
> +.B O_DIRECT
> +alignment restrictions,
> +for example the
>   .B XFS_IOC_DIOINFO
>   operation in
>   .BR xfsctl (3).
> +.B STATX_DIOALIGN
> +should be used instead when it is available.
>   .PP
> -Under Linux 2.4, transfer sizes, the alignment of the user buffer,
> -and the file offset must all be multiples of the logical block size
> -of the filesystem.
> -Since Linux 2.6.0, alignment to the logical block size of the
> -underlying storage (typically 512 bytes) suffices.
> -The logical block size can be determined using the
> +If none of the above is available,
> +then direct I/O support and alignment restrictions
> +can only be assumed from known characteristics of the filesystem,
> +the individual file,
> +the underlying storage device(s),
> +and the kernel version.
> +In Linux 2.4,
> +most block device based filesystems require that

I (actually I followed advise from someone else; credited in the commit) 
reworded "block device based filesystems" to "filesystems based on block 
devices", to avoid complex hyphenation rules.  If not reworded, it 
should have been something like "block-device\[en]based filesystems".

> +the file offset and the length and memory address of all I/O segments
> +be multiples of the filesystem block size
> +(typically 4096 bytes).
> +In Linux 2.6.0,
> +this was relaxed to the logical block size of the block device
> +(typically 512 bytes).
> +A block device's logical block size can be determined using the
>   .BR ioctl (2)
>   .B BLKSSZGET
>   operation or from the shell using the command:
> diff --git a/man2/statx.2 b/man2/statx.2
> index 2a85be7c0..84c35bdf3 100644
> --- a/man2/statx.2
> +++ b/man2/statx.2
> @@ -61,7 +61,12 @@ struct statx {
>          containing the filesystem where the file resides */
>       __u32 stx_dev_major;   /* Major ID */
>       __u32 stx_dev_minor;   /* Minor ID */
> +
>       __u64 stx_mnt_id;      /* Mount ID */
> +
> +    /* Direct I/O alignment restrictions */
> +    __u32 stx_dio_mem_align;
> +    __u32 stx_dio_offset_align;
>   };
>   .EE
>   .in
> @@ -247,6 +252,8 @@ STATX_BTIME	Want stx_btime
>   STATX_ALL	The same as STATX_BASIC_STATS | STATX_BTIME.
>   	It is deprecated and should not be used.
>   STATX_MNT_ID	Want stx_mnt_id (since Linux 5.8)
> +STATX_DIOALIGN	Want stx_dio_mem_align and stx_dio_offset_align
> +	(since Linux 6.1; support varies by filesystem)
>   .TE
>   .in
>   .PP
> @@ -407,6 +414,30 @@ This is the same number reported by
>   .BR name_to_handle_at (2)
>   and corresponds to the number in the first field in one of the records in
>   .IR /proc/self/mountinfo .
> +.TP
> +.I stx_dio_mem_align
> +The alignment (in bytes) required for user memory buffers for direct I/O
> +.RB ( O_DIRECT )
> +on this file,
> +or 0 if direct I/O is not supported on this file.
> +.IP
> +.B STATX_DIOALIGN
> +.RI ( stx_dio_mem_align
> +and
> +.IR stx_dio_offset_align )
> +is supported on block devices since Linux 6.1.
> +The support on regular files varies by filesystem;
> +it is supported by ext4, f2fs, and xfs since Linux 6.1.
> +.TP
> +.I stx_dio_offset_align
> +The alignment (in bytes) required for file offsets and I/O segment lengths
> +for direct I/O
> +.BR "" ( O_DIRECT )

s/.BR ""/.RB/

I guess you didn't see this one; as IIRC you fixed other identical cases.

> +on this file,
> +or 0 if direct I/O is not supported on this file.
> +This will only be nonzero if
> +.I stx_dio_mem_align
> +is nonzero, and vice versa.
>   .PP
>   For further information on the above fields, see
>   .BR inode (7).
> 
> base-commit: ab47278f252262dd9bd90f3386ffd7d8700fa25a

-- 
<http://www.alejandro-colomar.es/>

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists