| Message ID | 20240301164444.3799288-4-kernel@pankajraghav.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | enable bs > ps in XFS | expand |
On Fri, Mar 01, 2024 at 05:44:34PM +0100, Pankaj Raghav (Samsung) wrote: > +#define DEFINE_READAHEAD_ALIGNED(ractl, f, r, m, i) \ > + struct readahead_control ractl = { \ > + .file = f, \ > + .mapping = m, \ > + .ra = r, \ > + ._index = mapping_align_start_index(m, i), \ > + } My point was that you didn't need to do any of this. Look, I've tried to give constructive review, but I feel like I'm going to have to be blunt. There is no evidence of design or understanding in these patches or their commit messages. You don't have a coherent message about "These things have to be aligned; these things can be at arbitrary alignment". If you have thought about it, it doesn't show. Maybe you just need to go back over the patches and read them as a series, but it feels like "Oh, there's a hole here, patch it; another hole here, patch it" without thinking about what's going on and why. I want to help, but it feels like it'd be easier to do all the work myself at this point, and that's not good for me, and it's not good for you. So, let's start off: Is the index in ractl aligned or not, and why do you believe that's the right approach? And review each of the patches in this series with the answer to that question in mind because you are currently inconsistent.
On Fri, Mar 01, 2024 at 07:26:55PM +0000, Matthew Wilcox wrote: > On Fri, Mar 01, 2024 at 05:44:34PM +0100, Pankaj Raghav (Samsung) wrote: > > +#define DEFINE_READAHEAD_ALIGNED(ractl, f, r, m, i) \ > > + struct readahead_control ractl = { \ > > + .file = f, \ > > + .mapping = m, \ > > + .ra = r, \ > > + ._index = mapping_align_start_index(m, i), \ > > + } > > My point was that you didn't need to do any of this. > > Look, I've tried to give constructive review, but I feel like I'm going > to have to be blunt. There is no evidence of design or understanding > in these patches or their commit messages. You don't have a coherent > message about "These things have to be aligned; these things can be at > arbitrary alignment". If you have thought about it, it doesn't show. Don't you think you might be going off a bit much? I looked over these patches after we talked privately, and they looked pretty sensible to me... Yes, we _always_ want more thorough commit messages that properly explain the motivations for changes, but in my experience that's the thing that takes the longest to learn how to do well as an engineer... ease up abit. > So, let's start off: Is the index in ractl aligned or not, and why do > you believe that's the right approach? And review each of the patches > in this series with the answer to that question in mind because you are > currently inconsistent. ^ this is a real point though, DEFINE_READAHEAD_ALIGNED() feels off to me.
On Fri, Mar 01, 2024 at 07:26:55PM +0000, Matthew Wilcox wrote: > On Fri, Mar 01, 2024 at 05:44:34PM +0100, Pankaj Raghav (Samsung) wrote: > > +#define DEFINE_READAHEAD_ALIGNED(ractl, f, r, m, i) \ > > + struct readahead_control ractl = { \ > > + .file = f, \ > > + .mapping = m, \ > > + .ra = r, \ > > + ._index = mapping_align_start_index(m, i), \ > > + } > > My point was that you didn't need to do any of this. > Got it. I probably didn't understand your old comment properly. > Look, I've tried to give constructive review, but I feel like I'm going > to have to be blunt. There is no evidence of design or understanding > in these patches or their commit messages. You don't have a coherent > message about "These things have to be aligned; these things can be at > arbitrary alignment". If you have thought about it, it doesn't show. > > Maybe you just need to go back over the patches and read them as a series, > but it feels like "Oh, there's a hole here, patch it; another hole here, > patch it" without thinking about what's going on and why. > > I want to help, but it feels like it'd be easier to do all the work myself > at this point, and that's not good for me, and it's not good for you. > > So, let's start off: Is the index in ractl aligned or not, and why do > you believe that's the right approach? And review each of the patches > in this series with the answer to that question in mind because you are > currently inconsistent. Thanks for the feedback, and I get your comment about inconsistentency, especially in the part where we align the index probably in places where it doesn't even matter. As someone who is a bit new to the inner workings of the page cache, I was a bit unsure about choosing the right abstracation to enforce alignment. I am going through all the patches now based on your feedback and changing the commit messages to clarify the intent. -- Pankaj
On Fri, Mar 01, 2024 at 03:04:33PM -0500, Kent Overstreet wrote: > On Fri, Mar 01, 2024 at 07:26:55PM +0000, Matthew Wilcox wrote: > > On Fri, Mar 01, 2024 at 05:44:34PM +0100, Pankaj Raghav (Samsung) wrote: > > > +#define DEFINE_READAHEAD_ALIGNED(ractl, f, r, m, i) \ > > > + struct readahead_control ractl = { \ > > > + .file = f, \ > > > + .mapping = m, \ > > > + .ra = r, \ > > > + ._index = mapping_align_start_index(m, i), \ > > > + } > > > > My point was that you didn't need to do any of this. > > > > Look, I've tried to give constructive review, but I feel like I'm going > > to have to be blunt. There is no evidence of design or understanding > > in these patches or their commit messages. You don't have a coherent > > message about "These things have to be aligned; these things can be at > > arbitrary alignment". If you have thought about it, it doesn't show. > > Don't you think you might be going off a bit much? I looked over these > patches after we talked privately, and they looked pretty sensible to > me... > > Yes, we _always_ want more thorough commit messages that properly > explain the motivations for changes, but in my experience that's the > thing that takes the longest to learn how to do well as an engineer... > ease up abit. > > > So, let's start off: Is the index in ractl aligned or not, and why do > > you believe that's the right approach? And review each of the patches > > in this series with the answer to that question in mind because you are > > currently inconsistent. > > ^ this is a real point though, DEFINE_READAHEAD_ALIGNED() feels off to > me. Thanks Kent. I am going over the patches again and changing it based on the feedback.
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h index fc8eb9c94e9c..b3cf8ef89826 100644 --- a/include/linux/pagemap.h +++ b/include/linux/pagemap.h @@ -1328,6 +1328,14 @@ struct readahead_control { ._index = i, \ } +#define DEFINE_READAHEAD_ALIGNED(ractl, f, r, m, i) \ + struct readahead_control ractl = { \ + .file = f, \ + .mapping = m, \ + .ra = r, \ + ._index = mapping_align_start_index(m, i), \ + } + #define VM_READAHEAD_PAGES (SZ_128K / PAGE_SIZE) void page_cache_ra_unbounded(struct readahead_control *, @@ -1356,7 +1364,7 @@ void page_cache_sync_readahead(struct address_space *mapping, struct file_ra_state *ra, struct file *file, pgoff_t index, unsigned long req_count) { - DEFINE_READAHEAD(ractl, file, ra, mapping, index); + DEFINE_READAHEAD_ALIGNED(ractl, file, ra, mapping, index); page_cache_sync_ra(&ractl, req_count); } diff --git a/mm/filemap.c b/mm/filemap.c index 2b00442b9d19..96fe5c7fe094 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -2416,11 +2416,13 @@ static int filemap_update_page(struct kiocb *iocb, } static int filemap_create_folio(struct file *file, - struct address_space *mapping, pgoff_t index, + struct address_space *mapping, loff_t pos, struct folio_batch *fbatch) { struct folio *folio; int error; + unsigned int min_order = mapping_min_folio_order(mapping); + pgoff_t index; folio = filemap_alloc_folio(mapping_gfp_mask(mapping), 0); if (!folio) @@ -2440,6 +2442,8 @@ static int filemap_create_folio(struct file *file, * well to keep locking rules simple. */ filemap_invalidate_lock_shared(mapping); + /* index in PAGE units but aligned to min_order number of pages. */ + index = (pos >> (PAGE_SHIFT + min_order)) << min_order; error = filemap_add_folio(mapping, folio, index, mapping_gfp_constraint(mapping, GFP_KERNEL)); if (error == -EEXIST) @@ -2500,8 +2504,7 @@ static int filemap_get_pages(struct kiocb *iocb, size_t count, if (!folio_batch_count(fbatch)) { if (iocb->ki_flags & (IOCB_NOWAIT | IOCB_WAITQ)) return -EAGAIN; - err = filemap_create_folio(filp, mapping, - iocb->ki_pos >> PAGE_SHIFT, fbatch); + err = filemap_create_folio(filp, mapping, iocb->ki_pos, fbatch); if (err == AOP_TRUNCATED_PAGE) goto retry; return err; @@ -3093,7 +3096,7 @@ static struct file *do_sync_mmap_readahead(struct vm_fault *vmf) struct file *file = vmf->vma->vm_file; struct file_ra_state *ra = &file->f_ra; struct address_space *mapping = file->f_mapping; - DEFINE_READAHEAD(ractl, file, ra, mapping, vmf->pgoff); + DEFINE_READAHEAD_ALIGNED(ractl, file, ra, mapping, vmf->pgoff); struct file *fpin = NULL; unsigned long vm_flags = vmf->vma->vm_flags; unsigned int mmap_miss; @@ -3147,7 +3150,7 @@ static struct file *do_sync_mmap_readahead(struct vm_fault *vmf) ra->start = max_t(long, 0, vmf->pgoff - ra->ra_pages / 2); ra->size = ra->ra_pages; ra->async_size = ra->ra_pages / 4; - ractl._index = ra->start; + ractl._index = mapping_align_start_index(mapping, ra->start); page_cache_ra_order(&ractl, ra, 0); return fpin; } @@ -3162,7 +3165,7 @@ static struct file *do_async_mmap_readahead(struct vm_fault *vmf, { struct file *file = vmf->vma->vm_file; struct file_ra_state *ra = &file->f_ra; - DEFINE_READAHEAD(ractl, file, ra, file->f_mapping, vmf->pgoff); + DEFINE_READAHEAD_ALIGNED(ractl, file, ra, file->f_mapping, vmf->pgoff); struct file *fpin = NULL; unsigned int mmap_miss; @@ -3657,6 +3660,7 @@ static struct folio *do_read_cache_folio(struct address_space *mapping, struct folio *folio; int err; + index = mapping_align_start_index(mapping, index); if (!filler) filler = mapping->a_ops->read_folio; repeat: