* [PATCHSET 0/2] Avoid unnecessary indirect calls for bdev dio
@ 2021-12-03 15:38 Jens Axboe
2021-12-03 15:38 ` [PATCH 1/2] mm: move filemap_range_needs_writeback() into header Jens Axboe
2021-12-03 15:38 ` [PATCH 2/2] block: move direct_IO into our own read_iter handler Jens Axboe
0 siblings, 2 replies; 15+ messages in thread
From: Jens Axboe @ 2021-12-03 15:38 UTC (permalink / raw)
To: linux-block, linux-mm
Hi,
There really is no point to going through generic_file_read_iter() only
to call back into our own bdev dio handler. Move the logic into
bdev/fops.c instead and avoid both a function call and an indirect
function call.
--
Jens Axboe
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] mm: move filemap_range_needs_writeback() into header
2021-12-03 15:38 [PATCHSET 0/2] Avoid unnecessary indirect calls for bdev dio Jens Axboe
@ 2021-12-03 15:38 ` Jens Axboe
2021-12-03 16:16 ` Matthew Wilcox
2021-12-03 15:38 ` [PATCH 2/2] block: move direct_IO into our own read_iter handler Jens Axboe
1 sibling, 1 reply; 15+ messages in thread
From: Jens Axboe @ 2021-12-03 15:38 UTC (permalink / raw)
To: linux-block, linux-mm; +Cc: Jens Axboe
No functional changes in this patch, just in preparation for efficiently
calling this light function from the block O_DIRECT handling.
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
include/linux/fs.h | 35 +++++++++++++++++++++++++++++++++++
mm/filemap.c | 38 +++-----------------------------------
2 files changed, 38 insertions(+), 35 deletions(-)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index bbf812ce89a8..0cc4f5fd4cfe 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2845,6 +2845,41 @@ static inline int filemap_fdatawait(struct address_space *mapping)
return filemap_fdatawait_range(mapping, 0, LLONG_MAX);
}
+/* Returns true if writeback might be needed or already in progress. */
+static inline bool mapping_needs_writeback(struct address_space *mapping)
+{
+ return mapping->nrpages;
+}
+
+bool filemap_range_has_writeback(struct address_space *mapping,
+ loff_t start_byte, loff_t end_byte);
+
+/**
+ * filemap_range_needs_writeback - check if range potentially needs writeback
+ * @mapping: address space within which to check
+ * @start_byte: offset in bytes where the range starts
+ * @end_byte: offset in bytes where the range ends (inclusive)
+ *
+ * Find at least one page in the range supplied, usually used to check if
+ * direct writing in this range will trigger a writeback. Used by O_DIRECT
+ * read/write with IOCB_NOWAIT, to see if the caller needs to do
+ * filemap_write_and_wait_range() before proceeding.
+ *
+ * Return: %true if the caller should do filemap_write_and_wait_range() before
+ * doing O_DIRECT to a page in this range, %false otherwise.
+ */
+static inline bool filemap_range_needs_writeback(struct address_space *mapping,
+ loff_t start_byte,
+ loff_t end_byte)
+{
+ if (!mapping_needs_writeback(mapping))
+ return false;
+ if (!mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) &&
+ !mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK))
+ return false;
+ return filemap_range_has_writeback(mapping, start_byte, end_byte);
+}
+
extern bool filemap_range_has_page(struct address_space *, loff_t lstart,
loff_t lend);
extern bool filemap_range_needs_writeback(struct address_space *,
diff --git a/mm/filemap.c b/mm/filemap.c
index daa0e23a6ee6..65238440fa0a 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -640,14 +640,8 @@ int filemap_fdatawait_keep_errors(struct address_space *mapping)
}
EXPORT_SYMBOL(filemap_fdatawait_keep_errors);
-/* Returns true if writeback might be needed or already in progress. */
-static bool mapping_needs_writeback(struct address_space *mapping)
-{
- return mapping->nrpages;
-}
-
-static bool filemap_range_has_writeback(struct address_space *mapping,
- loff_t start_byte, loff_t end_byte)
+bool filemap_range_has_writeback(struct address_space *mapping,
+ loff_t start_byte, loff_t end_byte)
{
XA_STATE(xas, &mapping->i_pages, start_byte >> PAGE_SHIFT);
pgoff_t max = end_byte >> PAGE_SHIFT;
@@ -667,34 +661,8 @@ static bool filemap_range_has_writeback(struct address_space *mapping,
}
rcu_read_unlock();
return page != NULL;
-
-}
-
-/**
- * filemap_range_needs_writeback - check if range potentially needs writeback
- * @mapping: address space within which to check
- * @start_byte: offset in bytes where the range starts
- * @end_byte: offset in bytes where the range ends (inclusive)
- *
- * Find at least one page in the range supplied, usually used to check if
- * direct writing in this range will trigger a writeback. Used by O_DIRECT
- * read/write with IOCB_NOWAIT, to see if the caller needs to do
- * filemap_write_and_wait_range() before proceeding.
- *
- * Return: %true if the caller should do filemap_write_and_wait_range() before
- * doing O_DIRECT to a page in this range, %false otherwise.
- */
-bool filemap_range_needs_writeback(struct address_space *mapping,
- loff_t start_byte, loff_t end_byte)
-{
- if (!mapping_needs_writeback(mapping))
- return false;
- if (!mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) &&
- !mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK))
- return false;
- return filemap_range_has_writeback(mapping, start_byte, end_byte);
}
-EXPORT_SYMBOL_GPL(filemap_range_needs_writeback);
+EXPORT_SYMBOL_GPL(filemap_range_has_writeback);
/**
* filemap_write_and_wait_range - write out & wait on a file range
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/2] block: move direct_IO into our own read_iter handler
2021-12-03 15:38 [PATCHSET 0/2] Avoid unnecessary indirect calls for bdev dio Jens Axboe
2021-12-03 15:38 ` [PATCH 1/2] mm: move filemap_range_needs_writeback() into header Jens Axboe
@ 2021-12-03 15:38 ` Jens Axboe
2021-12-06 6:58 ` Christoph Hellwig
1 sibling, 1 reply; 15+ messages in thread
From: Jens Axboe @ 2021-12-03 15:38 UTC (permalink / raw)
To: linux-block, linux-mm; +Cc: Jens Axboe
Don't call into generic_file_read_iter() if we know it's O_DIRECT, just
set it up ourselves and call our own handler. This avoids an indirect call
for O_DIRECT.
Fall back to filemap_read() if we fail.
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
block/fops.c | 37 ++++++++++++++++++++++++++++++++-----
1 file changed, 32 insertions(+), 5 deletions(-)
diff --git a/block/fops.c b/block/fops.c
index 93bb5bf66f69..10015e1a5b01 100644
--- a/block/fops.c
+++ b/block/fops.c
@@ -566,21 +566,48 @@ static ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
{
struct block_device *bdev = iocb->ki_filp->private_data;
loff_t size = bdev_nr_bytes(bdev);
+ size_t count = iov_iter_count(to);
loff_t pos = iocb->ki_pos;
size_t shorted = 0;
- ssize_t ret;
+ ssize_t ret = 0;
- if (unlikely(pos + iov_iter_count(to) > size)) {
+ if (unlikely(pos + count > size)) {
if (pos >= size)
return 0;
size -= pos;
- if (iov_iter_count(to) > size) {
- shorted = iov_iter_count(to) - size;
+ if (count > size) {
+ shorted = count - size;
iov_iter_truncate(to, size);
}
}
- ret = generic_file_read_iter(iocb, to);
+ if (iocb->ki_flags & IOCB_DIRECT) {
+ struct address_space *mapping = iocb->ki_filp->f_mapping;
+
+ if (iocb->ki_flags & IOCB_NOWAIT) {
+ if (filemap_range_needs_writeback(mapping, iocb->ki_pos,
+ iocb->ki_pos + count - 1))
+ return -EAGAIN;
+ } else {
+ ret = filemap_write_and_wait_range(mapping,
+ iocb->ki_pos,
+ iocb->ki_pos + count - 1);
+ if (ret < 0)
+ return ret;
+ }
+
+ file_accessed(iocb->ki_filp);
+
+ ret = blkdev_direct_IO(iocb, to);
+ if (ret >= 0) {
+ iocb->ki_pos += ret;
+ count -= ret;
+ }
+ if (ret < 0 || !count)
+ return ret;
+ }
+
+ ret = filemap_read(iocb, to, ret);
if (unlikely(shorted))
iov_iter_reexpand(to, iov_iter_count(to) + shorted);
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] mm: move filemap_range_needs_writeback() into header
2021-12-03 15:38 ` [PATCH 1/2] mm: move filemap_range_needs_writeback() into header Jens Axboe
@ 2021-12-03 16:16 ` Matthew Wilcox
2021-12-03 16:24 ` Jens Axboe
0 siblings, 1 reply; 15+ messages in thread
From: Matthew Wilcox @ 2021-12-03 16:16 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, linux-mm
On Fri, Dec 03, 2021 at 08:38:28AM -0700, Jens Axboe wrote:
> +++ b/include/linux/fs.h
fs.h is the wrong place for these functions; they're pagecache
functionality, so they should be in pagemap.h.
> +/* Returns true if writeback might be needed or already in progress. */
> +static inline bool mapping_needs_writeback(struct address_space *mapping)
> +{
> + return mapping->nrpages;
> +}
I don't like this function -- mapping_needs_writeback says to me that it
tests a flag in mapping->flags. Plus, it does exactly the same thing as
!mapping_empty(), so perhaps ...
> +static inline bool filemap_range_needs_writeback(struct address_space *mapping,
> + loff_t start_byte,
> + loff_t end_byte)
> +{
> + if (!mapping_needs_writeback(mapping))
> + return false;
just make this
if (mapping_empty(mapping))
return false;
Other than that, no objections to making this static inline.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] mm: move filemap_range_needs_writeback() into header
2021-12-03 16:16 ` Matthew Wilcox
@ 2021-12-03 16:24 ` Jens Axboe
2021-12-03 16:31 ` Jens Axboe
0 siblings, 1 reply; 15+ messages in thread
From: Jens Axboe @ 2021-12-03 16:24 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: linux-block, linux-mm
On 12/3/21 9:16 AM, Matthew Wilcox wrote:
> On Fri, Dec 03, 2021 at 08:38:28AM -0700, Jens Axboe wrote:
>> +++ b/include/linux/fs.h
>
> fs.h is the wrong place for these functions; they're pagecache
> functionality, so they should be in pagemap.h.
>
>> +/* Returns true if writeback might be needed or already in progress. */
>> +static inline bool mapping_needs_writeback(struct address_space *mapping)
>> +{
>> + return mapping->nrpages;
>> +}
>
> I don't like this function -- mapping_needs_writeback says to me that it
> tests a flag in mapping->flags. Plus, it does exactly the same thing as
> !mapping_empty(), so perhaps ...
>
>> +static inline bool filemap_range_needs_writeback(struct address_space *mapping,
>> + loff_t start_byte,
>> + loff_t end_byte)
>> +{
>> + if (!mapping_needs_writeback(mapping))
>> + return false;
>
> just make this
> if (mapping_empty(mapping))
> return false;
>
> Other than that, no objections to making this static inline.
Good idea, I'll make that change.
--
Jens Axboe
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] mm: move filemap_range_needs_writeback() into header
2021-12-03 16:24 ` Jens Axboe
@ 2021-12-03 16:31 ` Jens Axboe
2021-12-03 16:35 ` Jens Axboe
0 siblings, 1 reply; 15+ messages in thread
From: Jens Axboe @ 2021-12-03 16:31 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: linux-block, linux-mm
On 12/3/21 9:24 AM, Jens Axboe wrote:
> On 12/3/21 9:16 AM, Matthew Wilcox wrote:
>> On Fri, Dec 03, 2021 at 08:38:28AM -0700, Jens Axboe wrote:
>>> +++ b/include/linux/fs.h
>>
>> fs.h is the wrong place for these functions; they're pagecache
>> functionality, so they should be in pagemap.h.
>>
>>> +/* Returns true if writeback might be needed or already in progress. */
>>> +static inline bool mapping_needs_writeback(struct address_space *mapping)
>>> +{
>>> + return mapping->nrpages;
>>> +}
>>
>> I don't like this function -- mapping_needs_writeback says to me that it
>> tests a flag in mapping->flags. Plus, it does exactly the same thing as
>> !mapping_empty(), so perhaps ...
>>
>>> +static inline bool filemap_range_needs_writeback(struct address_space *mapping,
>>> + loff_t start_byte,
>>> + loff_t end_byte)
>>> +{
>>> + if (!mapping_needs_writeback(mapping))
>>> + return false;
>>
>> just make this
>> if (mapping_empty(mapping))
>> return false;
>>
>> Other than that, no objections to making this static inline.
>
> Good idea, I'll make that change.
That does introduce a dependency from fs.h -> pagemap.h which isn't trivially
resolvable...
What if we just rename the above funciton to mapping_has_pages() or something
instead?
--
Jens Axboe
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] mm: move filemap_range_needs_writeback() into header
2021-12-03 16:31 ` Jens Axboe
@ 2021-12-03 16:35 ` Jens Axboe
2021-12-03 16:38 ` Jens Axboe
0 siblings, 1 reply; 15+ messages in thread
From: Jens Axboe @ 2021-12-03 16:35 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: linux-block, linux-mm
On 12/3/21 9:31 AM, Jens Axboe wrote:
> On 12/3/21 9:24 AM, Jens Axboe wrote:
>> On 12/3/21 9:16 AM, Matthew Wilcox wrote:
>>> On Fri, Dec 03, 2021 at 08:38:28AM -0700, Jens Axboe wrote:
>>>> +++ b/include/linux/fs.h
>>>
>>> fs.h is the wrong place for these functions; they're pagecache
>>> functionality, so they should be in pagemap.h.
>>>
>>>> +/* Returns true if writeback might be needed or already in progress. */
>>>> +static inline bool mapping_needs_writeback(struct address_space *mapping)
>>>> +{
>>>> + return mapping->nrpages;
>>>> +}
>>>
>>> I don't like this function -- mapping_needs_writeback says to me that it
>>> tests a flag in mapping->flags. Plus, it does exactly the same thing as
>>> !mapping_empty(), so perhaps ...
>>>
>>>> +static inline bool filemap_range_needs_writeback(struct address_space *mapping,
>>>> + loff_t start_byte,
>>>> + loff_t end_byte)
>>>> +{
>>>> + if (!mapping_needs_writeback(mapping))
>>>> + return false;
>>>
>>> just make this
>>> if (mapping_empty(mapping))
>>> return false;
>>>
>>> Other than that, no objections to making this static inline.
>>
>> Good idea, I'll make that change.
>
> That does introduce a dependency from fs.h -> pagemap.h which isn't trivially
> resolvable...
>
> What if we just rename the above funciton to mapping_has_pages() or something
> instead?
Or just drop the helper, to be honest. There are more tests for mapping->nrpages
right now than there are callers of this silly little helper.
--
Jens Axboe
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] mm: move filemap_range_needs_writeback() into header
2021-12-03 16:35 ` Jens Axboe
@ 2021-12-03 16:38 ` Jens Axboe
2021-12-03 17:46 ` Matthew Wilcox
0 siblings, 1 reply; 15+ messages in thread
From: Jens Axboe @ 2021-12-03 16:38 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: linux-block, linux-mm
On 12/3/21 9:35 AM, Jens Axboe wrote:
> On 12/3/21 9:31 AM, Jens Axboe wrote:
>> On 12/3/21 9:24 AM, Jens Axboe wrote:
>>> On 12/3/21 9:16 AM, Matthew Wilcox wrote:
>>>> On Fri, Dec 03, 2021 at 08:38:28AM -0700, Jens Axboe wrote:
>>>>> +++ b/include/linux/fs.h
>>>>
>>>> fs.h is the wrong place for these functions; they're pagecache
>>>> functionality, so they should be in pagemap.h.
>>>>
>>>>> +/* Returns true if writeback might be needed or already in progress. */
>>>>> +static inline bool mapping_needs_writeback(struct address_space *mapping)
>>>>> +{
>>>>> + return mapping->nrpages;
>>>>> +}
>>>>
>>>> I don't like this function -- mapping_needs_writeback says to me that it
>>>> tests a flag in mapping->flags. Plus, it does exactly the same thing as
>>>> !mapping_empty(), so perhaps ...
>>>>
>>>>> +static inline bool filemap_range_needs_writeback(struct address_space *mapping,
>>>>> + loff_t start_byte,
>>>>> + loff_t end_byte)
>>>>> +{
>>>>> + if (!mapping_needs_writeback(mapping))
>>>>> + return false;
>>>>
>>>> just make this
>>>> if (mapping_empty(mapping))
>>>> return false;
>>>>
>>>> Other than that, no objections to making this static inline.
>>>
>>> Good idea, I'll make that change.
>>
>> That does introduce a dependency from fs.h -> pagemap.h which isn't trivially
>> resolvable...
>>
>> What if we just rename the above funciton to mapping_has_pages() or something
>> instead?
>
> Or just drop the helper, to be honest. There are more tests for
> mapping->nrpages right now than there are callers of this silly little
> helper.
Like this:
commit 80d0d63df336376f53375c98703bcae0ec50d26b
Author: Jens Axboe <axboe@kernel.dk>
Date: Thu Oct 28 08:47:05 2021 -0600
mm: move filemap_range_needs_writeback() into header
No functional changes in this patch, just in preparation for efficiently
calling this light function from the block O_DIRECT handling.
Signed-off-by: Jens Axboe <axboe@kernel.dk>
diff --git a/include/linux/fs.h b/include/linux/fs.h
index bbf812ce89a8..11a37adc2520 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2845,6 +2845,35 @@ static inline int filemap_fdatawait(struct address_space *mapping)
return filemap_fdatawait_range(mapping, 0, LLONG_MAX);
}
+bool filemap_range_has_writeback(struct address_space *mapping,
+ loff_t start_byte, loff_t end_byte);
+
+/**
+ * filemap_range_needs_writeback - check if range potentially needs writeback
+ * @mapping: address space within which to check
+ * @start_byte: offset in bytes where the range starts
+ * @end_byte: offset in bytes where the range ends (inclusive)
+ *
+ * Find at least one page in the range supplied, usually used to check if
+ * direct writing in this range will trigger a writeback. Used by O_DIRECT
+ * read/write with IOCB_NOWAIT, to see if the caller needs to do
+ * filemap_write_and_wait_range() before proceeding.
+ *
+ * Return: %true if the caller should do filemap_write_and_wait_range() before
+ * doing O_DIRECT to a page in this range, %false otherwise.
+ */
+static inline bool filemap_range_needs_writeback(struct address_space *mapping,
+ loff_t start_byte,
+ loff_t end_byte)
+{
+ if (!mapping->nrpages)
+ return false;
+ if (!mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) &&
+ !mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK))
+ return false;
+ return filemap_range_has_writeback(mapping, start_byte, end_byte);
+}
+
extern bool filemap_range_has_page(struct address_space *, loff_t lstart,
loff_t lend);
extern bool filemap_range_needs_writeback(struct address_space *,
diff --git a/mm/filemap.c b/mm/filemap.c
index daa0e23a6ee6..655c9eec06b3 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -646,8 +646,8 @@ static bool mapping_needs_writeback(struct address_space *mapping)
return mapping->nrpages;
}
-static bool filemap_range_has_writeback(struct address_space *mapping,
- loff_t start_byte, loff_t end_byte)
+bool filemap_range_has_writeback(struct address_space *mapping,
+ loff_t start_byte, loff_t end_byte)
{
XA_STATE(xas, &mapping->i_pages, start_byte >> PAGE_SHIFT);
pgoff_t max = end_byte >> PAGE_SHIFT;
@@ -667,34 +667,8 @@ static bool filemap_range_has_writeback(struct address_space *mapping,
}
rcu_read_unlock();
return page != NULL;
-
-}
-
-/**
- * filemap_range_needs_writeback - check if range potentially needs writeback
- * @mapping: address space within which to check
- * @start_byte: offset in bytes where the range starts
- * @end_byte: offset in bytes where the range ends (inclusive)
- *
- * Find at least one page in the range supplied, usually used to check if
- * direct writing in this range will trigger a writeback. Used by O_DIRECT
- * read/write with IOCB_NOWAIT, to see if the caller needs to do
- * filemap_write_and_wait_range() before proceeding.
- *
- * Return: %true if the caller should do filemap_write_and_wait_range() before
- * doing O_DIRECT to a page in this range, %false otherwise.
- */
-bool filemap_range_needs_writeback(struct address_space *mapping,
- loff_t start_byte, loff_t end_byte)
-{
- if (!mapping_needs_writeback(mapping))
- return false;
- if (!mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) &&
- !mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK))
- return false;
- return filemap_range_has_writeback(mapping, start_byte, end_byte);
}
-EXPORT_SYMBOL_GPL(filemap_range_needs_writeback);
+EXPORT_SYMBOL_GPL(filemap_range_has_writeback);
/**
* filemap_write_and_wait_range - write out & wait on a file range
--
Jens Axboe
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] mm: move filemap_range_needs_writeback() into header
2021-12-03 16:38 ` Jens Axboe
@ 2021-12-03 17:46 ` Matthew Wilcox
2021-12-03 17:57 ` Jens Axboe
0 siblings, 1 reply; 15+ messages in thread
From: Matthew Wilcox @ 2021-12-03 17:46 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, linux-mm
On Fri, Dec 03, 2021 at 09:38:25AM -0700, Jens Axboe wrote:
> On 12/3/21 9:35 AM, Jens Axboe wrote:
> > On 12/3/21 9:31 AM, Jens Axboe wrote:
> >> On 12/3/21 9:24 AM, Jens Axboe wrote:
> >>> On 12/3/21 9:16 AM, Matthew Wilcox wrote:
> >>>> On Fri, Dec 03, 2021 at 08:38:28AM -0700, Jens Axboe wrote:
> >>>>> +++ b/include/linux/fs.h
> >>>>
> >>>> fs.h is the wrong place for these functions; they're pagecache
> >>>> functionality, so they should be in pagemap.h.
I think you missed this ^^^
> >> That does introduce a dependency from fs.h -> pagemap.h which isn't trivially
> >> resolvable...
> >>
> >> What if we just rename the above funciton to mapping_has_pages() or something
> >> instead?
> >
> > Or just drop the helper, to be honest. There are more tests for
> > mapping->nrpages right now than there are callers of this silly little
> > helper.
>
> Like this:
I'm happy with this, if you just move it to pagemap.h
>
> commit 80d0d63df336376f53375c98703bcae0ec50d26b
> Author: Jens Axboe <axboe@kernel.dk>
> Date: Thu Oct 28 08:47:05 2021 -0600
>
> mm: move filemap_range_needs_writeback() into header
>
> No functional changes in this patch, just in preparation for efficiently
> calling this light function from the block O_DIRECT handling.
>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index bbf812ce89a8..11a37adc2520 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2845,6 +2845,35 @@ static inline int filemap_fdatawait(struct address_space *mapping)
> return filemap_fdatawait_range(mapping, 0, LLONG_MAX);
> }
>
> +bool filemap_range_has_writeback(struct address_space *mapping,
> + loff_t start_byte, loff_t end_byte);
> +
> +/**
> + * filemap_range_needs_writeback - check if range potentially needs writeback
> + * @mapping: address space within which to check
> + * @start_byte: offset in bytes where the range starts
> + * @end_byte: offset in bytes where the range ends (inclusive)
> + *
> + * Find at least one page in the range supplied, usually used to check if
> + * direct writing in this range will trigger a writeback. Used by O_DIRECT
> + * read/write with IOCB_NOWAIT, to see if the caller needs to do
> + * filemap_write_and_wait_range() before proceeding.
> + *
> + * Return: %true if the caller should do filemap_write_and_wait_range() before
> + * doing O_DIRECT to a page in this range, %false otherwise.
> + */
> +static inline bool filemap_range_needs_writeback(struct address_space *mapping,
> + loff_t start_byte,
> + loff_t end_byte)
> +{
> + if (!mapping->nrpages)
> + return false;
> + if (!mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) &&
> + !mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK))
> + return false;
> + return filemap_range_has_writeback(mapping, start_byte, end_byte);
> +}
> +
> extern bool filemap_range_has_page(struct address_space *, loff_t lstart,
> loff_t lend);
> extern bool filemap_range_needs_writeback(struct address_space *,
> diff --git a/mm/filemap.c b/mm/filemap.c
> index daa0e23a6ee6..655c9eec06b3 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -646,8 +646,8 @@ static bool mapping_needs_writeback(struct address_space *mapping)
> return mapping->nrpages;
> }
>
> -static bool filemap_range_has_writeback(struct address_space *mapping,
> - loff_t start_byte, loff_t end_byte)
> +bool filemap_range_has_writeback(struct address_space *mapping,
> + loff_t start_byte, loff_t end_byte)
> {
> XA_STATE(xas, &mapping->i_pages, start_byte >> PAGE_SHIFT);
> pgoff_t max = end_byte >> PAGE_SHIFT;
> @@ -667,34 +667,8 @@ static bool filemap_range_has_writeback(struct address_space *mapping,
> }
> rcu_read_unlock();
> return page != NULL;
> -
> -}
> -
> -/**
> - * filemap_range_needs_writeback - check if range potentially needs writeback
> - * @mapping: address space within which to check
> - * @start_byte: offset in bytes where the range starts
> - * @end_byte: offset in bytes where the range ends (inclusive)
> - *
> - * Find at least one page in the range supplied, usually used to check if
> - * direct writing in this range will trigger a writeback. Used by O_DIRECT
> - * read/write with IOCB_NOWAIT, to see if the caller needs to do
> - * filemap_write_and_wait_range() before proceeding.
> - *
> - * Return: %true if the caller should do filemap_write_and_wait_range() before
> - * doing O_DIRECT to a page in this range, %false otherwise.
> - */
> -bool filemap_range_needs_writeback(struct address_space *mapping,
> - loff_t start_byte, loff_t end_byte)
> -{
> - if (!mapping_needs_writeback(mapping))
> - return false;
> - if (!mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) &&
> - !mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK))
> - return false;
> - return filemap_range_has_writeback(mapping, start_byte, end_byte);
> }
> -EXPORT_SYMBOL_GPL(filemap_range_needs_writeback);
> +EXPORT_SYMBOL_GPL(filemap_range_has_writeback);
>
> /**
> * filemap_write_and_wait_range - write out & wait on a file range
>
>
> --
> Jens Axboe
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] mm: move filemap_range_needs_writeback() into header
2021-12-03 17:46 ` Matthew Wilcox
@ 2021-12-03 17:57 ` Jens Axboe
2021-12-03 18:01 ` Jens Axboe
0 siblings, 1 reply; 15+ messages in thread
From: Jens Axboe @ 2021-12-03 17:57 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: linux-block, linux-mm
On 12/3/21 10:46 AM, Matthew Wilcox wrote:
> On Fri, Dec 03, 2021 at 09:38:25AM -0700, Jens Axboe wrote:
>> On 12/3/21 9:35 AM, Jens Axboe wrote:
>>> On 12/3/21 9:31 AM, Jens Axboe wrote:
>>>> On 12/3/21 9:24 AM, Jens Axboe wrote:
>>>>> On 12/3/21 9:16 AM, Matthew Wilcox wrote:
>>>>>> On Fri, Dec 03, 2021 at 08:38:28AM -0700, Jens Axboe wrote:
>>>>>>> +++ b/include/linux/fs.h
>>>>>>
>>>>>> fs.h is the wrong place for these functions; they're pagecache
>>>>>> functionality, so they should be in pagemap.h.
>
> I think you missed this ^^^
I did... That would mean moving some more of the declarations too. Prep patch
or just part of this patch?
>>>> That does introduce a dependency from fs.h -> pagemap.h which isn't trivially
>>>> resolvable...
>>>>
>>>> What if we just rename the above funciton to mapping_has_pages() or something
>>>> instead?
>>>
>>> Or just drop the helper, to be honest. There are more tests for
>>> mapping->nrpages right now than there are callers of this silly little
>>> helper.
>>
>> Like this:
>
> I'm happy with this, if you just move it to pagemap.h
OK, I'll try it out.
--
Jens Axboe
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] mm: move filemap_range_needs_writeback() into header
2021-12-03 17:57 ` Jens Axboe
@ 2021-12-03 18:01 ` Jens Axboe
2021-12-03 18:14 ` Matthew Wilcox
0 siblings, 1 reply; 15+ messages in thread
From: Jens Axboe @ 2021-12-03 18:01 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: linux-block, linux-mm
On 12/3/21 10:57 AM, Jens Axboe wrote:
>> I'm happy with this, if you just move it to pagemap.h
>
> OK, I'll try it out.
Wasn't too bad at all, actually just highlighted that I missed removing
the previous declaration of filemap_range_needs_writeback() in fs.h
I'll do a full compile and test, but this seems sane.
commit 63c6b3846b77041d239d5b5b5a907b5c82a21c4c
Author: Jens Axboe <axboe@kernel.dk>
Date: Thu Oct 28 08:47:05 2021 -0600
mm: move filemap_range_needs_writeback() into header
No functional changes in this patch, just in preparation for efficiently
calling this light function from the block O_DIRECT handling.
Signed-off-by: Jens Axboe <axboe@kernel.dk>
diff --git a/include/linux/fs.h b/include/linux/fs.h
index bbf812ce89a8..6b8dc1a78df6 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2847,8 +2847,6 @@ static inline int filemap_fdatawait(struct address_space *mapping)
extern bool filemap_range_has_page(struct address_space *, loff_t lstart,
loff_t lend);
-extern bool filemap_range_needs_writeback(struct address_space *,
- loff_t lstart, loff_t lend);
extern int filemap_write_and_wait_range(struct address_space *mapping,
loff_t lstart, loff_t lend);
extern int __filemap_fdatawrite_range(struct address_space *mapping,
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 605246452305..274a0710f2c5 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -963,6 +963,35 @@ static inline int add_to_page_cache(struct page *page,
int __filemap_add_folio(struct address_space *mapping, struct folio *folio,
pgoff_t index, gfp_t gfp, void **shadowp);
+bool filemap_range_has_writeback(struct address_space *mapping,
+ loff_t start_byte, loff_t end_byte);
+
+/**
+ * filemap_range_needs_writeback - check if range potentially needs writeback
+ * @mapping: address space within which to check
+ * @start_byte: offset in bytes where the range starts
+ * @end_byte: offset in bytes where the range ends (inclusive)
+ *
+ * Find at least one page in the range supplied, usually used to check if
+ * direct writing in this range will trigger a writeback. Used by O_DIRECT
+ * read/write with IOCB_NOWAIT, to see if the caller needs to do
+ * filemap_write_and_wait_range() before proceeding.
+ *
+ * Return: %true if the caller should do filemap_write_and_wait_range() before
+ * doing O_DIRECT to a page in this range, %false otherwise.
+ */
+static inline bool filemap_range_needs_writeback(struct address_space *mapping,
+ loff_t start_byte,
+ loff_t end_byte)
+{
+ if (!mapping->nrpages)
+ return false;
+ if (!mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) &&
+ !mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK))
+ return false;
+ return filemap_range_has_writeback(mapping, start_byte, end_byte);
+}
+
/**
* struct readahead_control - Describes a readahead request.
*
diff --git a/mm/filemap.c b/mm/filemap.c
index daa0e23a6ee6..655c9eec06b3 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -646,8 +646,8 @@ static bool mapping_needs_writeback(struct address_space *mapping)
return mapping->nrpages;
}
-static bool filemap_range_has_writeback(struct address_space *mapping,
- loff_t start_byte, loff_t end_byte)
+bool filemap_range_has_writeback(struct address_space *mapping,
+ loff_t start_byte, loff_t end_byte)
{
XA_STATE(xas, &mapping->i_pages, start_byte >> PAGE_SHIFT);
pgoff_t max = end_byte >> PAGE_SHIFT;
@@ -667,34 +667,8 @@ static bool filemap_range_has_writeback(struct address_space *mapping,
}
rcu_read_unlock();
return page != NULL;
-
-}
-
-/**
- * filemap_range_needs_writeback - check if range potentially needs writeback
- * @mapping: address space within which to check
- * @start_byte: offset in bytes where the range starts
- * @end_byte: offset in bytes where the range ends (inclusive)
- *
- * Find at least one page in the range supplied, usually used to check if
- * direct writing in this range will trigger a writeback. Used by O_DIRECT
- * read/write with IOCB_NOWAIT, to see if the caller needs to do
- * filemap_write_and_wait_range() before proceeding.
- *
- * Return: %true if the caller should do filemap_write_and_wait_range() before
- * doing O_DIRECT to a page in this range, %false otherwise.
- */
-bool filemap_range_needs_writeback(struct address_space *mapping,
- loff_t start_byte, loff_t end_byte)
-{
- if (!mapping_needs_writeback(mapping))
- return false;
- if (!mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) &&
- !mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK))
- return false;
- return filemap_range_has_writeback(mapping, start_byte, end_byte);
}
-EXPORT_SYMBOL_GPL(filemap_range_needs_writeback);
+EXPORT_SYMBOL_GPL(filemap_range_has_writeback);
/**
* filemap_write_and_wait_range - write out & wait on a file range
--
Jens Axboe
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] mm: move filemap_range_needs_writeback() into header
2021-12-03 18:01 ` Jens Axboe
@ 2021-12-03 18:14 ` Matthew Wilcox
2021-12-03 19:09 ` Jens Axboe
0 siblings, 1 reply; 15+ messages in thread
From: Matthew Wilcox @ 2021-12-03 18:14 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, linux-mm
On Fri, Dec 03, 2021 at 11:01:14AM -0700, Jens Axboe wrote:
> On 12/3/21 10:57 AM, Jens Axboe wrote:
> >> I'm happy with this, if you just move it to pagemap.h
> >
> > OK, I'll try it out.
>
> Wasn't too bad at all, actually just highlighted that I missed removing
> the previous declaration of filemap_range_needs_writeback() in fs.h
> I'll do a full compile and test, but this seems sane.
>
> commit 63c6b3846b77041d239d5b5b5a907b5c82a21c4c
> Author: Jens Axboe <axboe@kernel.dk>
> Date: Thu Oct 28 08:47:05 2021 -0600
>
> mm: move filemap_range_needs_writeback() into header
>
> No functional changes in this patch, just in preparation for efficiently
> calling this light function from the block O_DIRECT handling.
>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index bbf812ce89a8..6b8dc1a78df6 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2847,8 +2847,6 @@ static inline int filemap_fdatawait(struct address_space *mapping)
>
> extern bool filemap_range_has_page(struct address_space *, loff_t lstart,
> loff_t lend);
> -extern bool filemap_range_needs_writeback(struct address_space *,
> - loff_t lstart, loff_t lend);
> extern int filemap_write_and_wait_range(struct address_space *mapping,
> loff_t lstart, loff_t lend);
> extern int __filemap_fdatawrite_range(struct address_space *mapping,
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index 605246452305..274a0710f2c5 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -963,6 +963,35 @@ static inline int add_to_page_cache(struct page *page,
> int __filemap_add_folio(struct address_space *mapping, struct folio *folio,
> pgoff_t index, gfp_t gfp, void **shadowp);
>
> +bool filemap_range_has_writeback(struct address_space *mapping,
> + loff_t start_byte, loff_t end_byte);
> +
> +/**
> + * filemap_range_needs_writeback - check if range potentially needs writeback
> + * @mapping: address space within which to check
> + * @start_byte: offset in bytes where the range starts
> + * @end_byte: offset in bytes where the range ends (inclusive)
> + *
> + * Find at least one page in the range supplied, usually used to check if
> + * direct writing in this range will trigger a writeback. Used by O_DIRECT
> + * read/write with IOCB_NOWAIT, to see if the caller needs to do
> + * filemap_write_and_wait_range() before proceeding.
> + *
> + * Return: %true if the caller should do filemap_write_and_wait_range() before
> + * doing O_DIRECT to a page in this range, %false otherwise.
> + */
> +static inline bool filemap_range_needs_writeback(struct address_space *mapping,
> + loff_t start_byte,
> + loff_t end_byte)
> +{
> + if (!mapping->nrpages)
> + return false;
> + if (!mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) &&
> + !mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK))
> + return false;
> + return filemap_range_has_writeback(mapping, start_byte, end_byte);
> +}
> +
> /**
> * struct readahead_control - Describes a readahead request.
> *
> diff --git a/mm/filemap.c b/mm/filemap.c
> index daa0e23a6ee6..655c9eec06b3 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -646,8 +646,8 @@ static bool mapping_needs_writeback(struct address_space *mapping)
> return mapping->nrpages;
> }
>
> -static bool filemap_range_has_writeback(struct address_space *mapping,
> - loff_t start_byte, loff_t end_byte)
> +bool filemap_range_has_writeback(struct address_space *mapping,
> + loff_t start_byte, loff_t end_byte)
> {
> XA_STATE(xas, &mapping->i_pages, start_byte >> PAGE_SHIFT);
> pgoff_t max = end_byte >> PAGE_SHIFT;
> @@ -667,34 +667,8 @@ static bool filemap_range_has_writeback(struct address_space *mapping,
> }
> rcu_read_unlock();
> return page != NULL;
> -
> -}
> -
> -/**
> - * filemap_range_needs_writeback - check if range potentially needs writeback
> - * @mapping: address space within which to check
> - * @start_byte: offset in bytes where the range starts
> - * @end_byte: offset in bytes where the range ends (inclusive)
> - *
> - * Find at least one page in the range supplied, usually used to check if
> - * direct writing in this range will trigger a writeback. Used by O_DIRECT
> - * read/write with IOCB_NOWAIT, to see if the caller needs to do
> - * filemap_write_and_wait_range() before proceeding.
> - *
> - * Return: %true if the caller should do filemap_write_and_wait_range() before
> - * doing O_DIRECT to a page in this range, %false otherwise.
> - */
> -bool filemap_range_needs_writeback(struct address_space *mapping,
> - loff_t start_byte, loff_t end_byte)
> -{
> - if (!mapping_needs_writeback(mapping))
> - return false;
> - if (!mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) &&
> - !mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK))
> - return false;
> - return filemap_range_has_writeback(mapping, start_byte, end_byte);
> }
> -EXPORT_SYMBOL_GPL(filemap_range_needs_writeback);
> +EXPORT_SYMBOL_GPL(filemap_range_has_writeback);
>
> /**
> * filemap_write_and_wait_range - write out & wait on a file range
>
> --
> Jens Axboe
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] mm: move filemap_range_needs_writeback() into header
2021-12-03 18:14 ` Matthew Wilcox
@ 2021-12-03 19:09 ` Jens Axboe
0 siblings, 0 replies; 15+ messages in thread
From: Jens Axboe @ 2021-12-03 19:09 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: linux-block, linux-mm
On 12/3/21 11:14 AM, Matthew Wilcox wrote:
> On Fri, Dec 03, 2021 at 11:01:14AM -0700, Jens Axboe wrote:
>> On 12/3/21 10:57 AM, Jens Axboe wrote:
>>>> I'm happy with this, if you just move it to pagemap.h
>>>
>>> OK, I'll try it out.
>>
>> Wasn't too bad at all, actually just highlighted that I missed removing
>> the previous declaration of filemap_range_needs_writeback() in fs.h
>> I'll do a full compile and test, but this seems sane.
>>
>> commit 63c6b3846b77041d239d5b5b5a907b5c82a21c4c
>> Author: Jens Axboe <axboe@kernel.dk>
>> Date: Thu Oct 28 08:47:05 2021 -0600
>>
>> mm: move filemap_range_needs_writeback() into header
>>
>> No functional changes in this patch, just in preparation for efficiently
>> calling this light function from the block O_DIRECT handling.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>
> Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Great, thanks for the review Willy!
--
Jens Axboe
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] block: move direct_IO into our own read_iter handler
2021-12-03 15:38 ` [PATCH 2/2] block: move direct_IO into our own read_iter handler Jens Axboe
@ 2021-12-06 6:58 ` Christoph Hellwig
2021-12-06 16:33 ` Jens Axboe
0 siblings, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2021-12-06 6:58 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, linux-mm
On Fri, Dec 03, 2021 at 08:38:29AM -0700, Jens Axboe wrote:
> Don't call into generic_file_read_iter() if we know it's O_DIRECT, just
> set it up ourselves and call our own handler. This avoids an indirect call
> for O_DIRECT.
>
> Fall back to filemap_read() if we fail.
Please also do it for the write side, having a partial ->direct_IO is a
really bad idea.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] block: move direct_IO into our own read_iter handler
2021-12-06 6:58 ` Christoph Hellwig
@ 2021-12-06 16:33 ` Jens Axboe
0 siblings, 0 replies; 15+ messages in thread
From: Jens Axboe @ 2021-12-06 16:33 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-block, linux-mm
On 12/5/21 11:58 PM, Christoph Hellwig wrote:
> On Fri, Dec 03, 2021 at 08:38:29AM -0700, Jens Axboe wrote:
>> Don't call into generic_file_read_iter() if we know it's O_DIRECT, just
>> set it up ourselves and call our own handler. This avoids an indirect call
>> for O_DIRECT.
>>
>> Fall back to filemap_read() if we fail.
>
> Please also do it for the write side, having a partial ->direct_IO is a
> really bad idea.
Sure, I'll do the write side as well.
--
Jens Axboe
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-12-06 16:33 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-03 15:38 [PATCHSET 0/2] Avoid unnecessary indirect calls for bdev dio Jens Axboe
2021-12-03 15:38 ` [PATCH 1/2] mm: move filemap_range_needs_writeback() into header Jens Axboe
2021-12-03 16:16 ` Matthew Wilcox
2021-12-03 16:24 ` Jens Axboe
2021-12-03 16:31 ` Jens Axboe
2021-12-03 16:35 ` Jens Axboe
2021-12-03 16:38 ` Jens Axboe
2021-12-03 17:46 ` Matthew Wilcox
2021-12-03 17:57 ` Jens Axboe
2021-12-03 18:01 ` Jens Axboe
2021-12-03 18:14 ` Matthew Wilcox
2021-12-03 19:09 ` Jens Axboe
2021-12-03 15:38 ` [PATCH 2/2] block: move direct_IO into our own read_iter handler Jens Axboe
2021-12-06 6:58 ` Christoph Hellwig
2021-12-06 16:33 ` Jens Axboe
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.