From: Mike Kravetz <mike.kravetz@oracle.com>
To: Aristeu Rozanski <aris@ruivo.org>
Cc: Matthew Wilcox <willy@infradead.org>,
Aneesh Kumar K V <aneesh.kumar@linux.ibm.com>,
David Howells <dhowells@redhat.com>,
linux-mm@kvack.org, npiggin@gmail.com,
linux-fsdevel@vger.kernel.org, akpm@linux-foundation.org,
linuxppc-dev@lists.ozlabs.org, Al Viro <viro@zeniv.linux.org.uk>
Subject: Re: [RFC PATCH] fs/hugetlb: Fix UBSAN warning reported on hugetlb
Date: Wed, 26 Oct 2022 11:11:14 -0700 [thread overview]
Message-ID: <Y1l4Qr33LmTzvrhp@monkey> (raw)
In-Reply-To: <Y1lJNepa22oMZ3tR@cathedrallabs.org>
On 10/26/22 10:50, Aristeu Rozanski wrote:
> On Thu, Sep 08, 2022 at 10:29:59PM +0530, Aneesh Kumar K V wrote:
> > On 9/8/22 10:23 PM, Matthew Wilcox wrote:
> > > On Thu, Sep 08, 2022 at 12:56:59PM +0530, Aneesh Kumar K.V wrote:
> > >> +++ b/fs/dax.c
> > >> @@ -1304,7 +1304,7 @@ EXPORT_SYMBOL_GPL(dax_zero_range);
> > >> int dax_truncate_page(struct inode *inode, loff_t pos, bool *did_zero,
> > >> const struct iomap_ops *ops)
> > >> {
> > >> - unsigned int blocksize = i_blocksize(inode);
> > >> + size_t blocksize = i_blocksize(inode);
> > >> unsigned int off = pos & (blocksize - 1);
> > >
> > > If blocksize is larger than 4GB, then off also needs to be size_t.
> > >
> > >> +++ b/fs/iomap/buffered-io.c
> > >> @@ -955,7 +955,7 @@ int
> > >> iomap_truncate_page(struct inode *inode, loff_t pos, bool *did_zero,
> > >> const struct iomap_ops *ops)
> > >> {
> > >> - unsigned int blocksize = i_blocksize(inode);
> > >> + size_t blocksize = i_blocksize(inode);
> > >> unsigned int off = pos & (blocksize - 1);
> > >
> > > Ditto.
> > >
> > > (maybe there are others; I didn't check closely)
> >
> > Thanks. will check those.
> >
> > Any feedback on statx? Should we really fix that?
> >
> > I am still not clear why we chose to set blocksize = pagesize for hugetlbfs.
> > Was that done to enable application find the hugetlb pagesize via stat()?
>
> I'd like to know that as well. It'd be easier to just limit the hugetlbfs max
> blocksize to 4GB. It's very unlikely anything else will use such large
> blocksizes and having to introduce new user interfaces for it doesn't sound
> right.
I was not around hugetlbfs when the decision was made to set 'blocksize =
pagesize'. However, I must say that it does seem to make sense as you
can only add or remove entire hugetlb pages from a hugetlbfs file. So,
the hugetlb page size does seem to correspond to the meaning of filesystem
blocksize.
Does any application code make use of this? I can not make a guess.
--
Mike Kravetz
WARNING: multiple messages have this Message-ID (diff)
From: Mike Kravetz <mike.kravetz@oracle.com>
To: Aristeu Rozanski <aris@ruivo.org>
Cc: Aneesh Kumar K V <aneesh.kumar@linux.ibm.com>,
Matthew Wilcox <willy@infradead.org>,
linuxppc-dev@lists.ozlabs.org, mpe@ellerman.id.au,
npiggin@gmail.com, christophe.leroy@csgroup.eu,
linux-mm@kvack.org, akpm@linux-foundation.org,
David Howells <dhowells@redhat.com>,
linux-fsdevel@vger.kernel.org, Al Viro <viro@zeniv.linux.org.uk>
Subject: Re: [RFC PATCH] fs/hugetlb: Fix UBSAN warning reported on hugetlb
Date: Wed, 26 Oct 2022 11:11:14 -0700 [thread overview]
Message-ID: <Y1l4Qr33LmTzvrhp@monkey> (raw)
In-Reply-To: <Y1lJNepa22oMZ3tR@cathedrallabs.org>
On 10/26/22 10:50, Aristeu Rozanski wrote:
> On Thu, Sep 08, 2022 at 10:29:59PM +0530, Aneesh Kumar K V wrote:
> > On 9/8/22 10:23 PM, Matthew Wilcox wrote:
> > > On Thu, Sep 08, 2022 at 12:56:59PM +0530, Aneesh Kumar K.V wrote:
> > >> +++ b/fs/dax.c
> > >> @@ -1304,7 +1304,7 @@ EXPORT_SYMBOL_GPL(dax_zero_range);
> > >> int dax_truncate_page(struct inode *inode, loff_t pos, bool *did_zero,
> > >> const struct iomap_ops *ops)
> > >> {
> > >> - unsigned int blocksize = i_blocksize(inode);
> > >> + size_t blocksize = i_blocksize(inode);
> > >> unsigned int off = pos & (blocksize - 1);
> > >
> > > If blocksize is larger than 4GB, then off also needs to be size_t.
> > >
> > >> +++ b/fs/iomap/buffered-io.c
> > >> @@ -955,7 +955,7 @@ int
> > >> iomap_truncate_page(struct inode *inode, loff_t pos, bool *did_zero,
> > >> const struct iomap_ops *ops)
> > >> {
> > >> - unsigned int blocksize = i_blocksize(inode);
> > >> + size_t blocksize = i_blocksize(inode);
> > >> unsigned int off = pos & (blocksize - 1);
> > >
> > > Ditto.
> > >
> > > (maybe there are others; I didn't check closely)
> >
> > Thanks. will check those.
> >
> > Any feedback on statx? Should we really fix that?
> >
> > I am still not clear why we chose to set blocksize = pagesize for hugetlbfs.
> > Was that done to enable application find the hugetlb pagesize via stat()?
>
> I'd like to know that as well. It'd be easier to just limit the hugetlbfs max
> blocksize to 4GB. It's very unlikely anything else will use such large
> blocksizes and having to introduce new user interfaces for it doesn't sound
> right.
I was not around hugetlbfs when the decision was made to set 'blocksize =
pagesize'. However, I must say that it does seem to make sense as you
can only add or remove entire hugetlb pages from a hugetlbfs file. So,
the hugetlb page size does seem to correspond to the meaning of filesystem
blocksize.
Does any application code make use of this? I can not make a guess.
--
Mike Kravetz
next prev parent reply other threads:[~2022-10-26 19:40 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-08 7:26 [RFC PATCH] fs/hugetlb: Fix UBSAN warning reported on hugetlb Aneesh Kumar K.V
2022-09-08 7:26 ` Aneesh Kumar K.V
2022-09-08 15:21 ` kernel test robot
2022-09-08 16:53 ` Matthew Wilcox
2022-09-08 16:53 ` Matthew Wilcox
2022-09-08 16:59 ` Aneesh Kumar K V
2022-09-08 16:59 ` Aneesh Kumar K V
2022-10-26 14:50 ` Aristeu Rozanski
2022-10-26 18:11 ` Mike Kravetz [this message]
2022-10-26 18:11 ` Mike Kravetz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y1l4Qr33LmTzvrhp@monkey \
--to=mike.kravetz@oracle.com \
--cc=akpm@linux-foundation.org \
--cc=aneesh.kumar@linux.ibm.com \
--cc=aris@ruivo.org \
--cc=dhowells@redhat.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=npiggin@gmail.com \
--cc=viro@zeniv.linux.org.uk \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.