* [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
* 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
* [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 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:34 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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).