On Thu, May 20, 2021 at 02:58:58PM -0700, Darrick J. Wong wrote:
> On Tue, May 18, 2021 at 07:38:01AM -0400, Brian Foster wrote:
> > On Mon, May 17, 2021 at 06:54:34PM +0100, Matthew Wilcox wrote:
> > > On Mon, May 17, 2021 at 01:17:20PM -0400, Brian Foster wrote:
> > > > @@ -1084,9 +1084,12 @@ iomap_finish_ioend(struct iomap_ioend *ioend, int error)
> > > > next = bio->bi_private;
> > > >
> > > > /* walk each page on bio, ending page IO on them */
> > > > - bio_for_each_segment_all(bv, bio, iter_all)
> > > > + bio_for_each_segment_all(bv, bio, iter_all) {
> > > > iomap_finish_page_writeback(inode, bv->bv_page, error,
> > > > bv->bv_len);
> > > > + if (!atomic)
> > > > + cond_resched();
> > > > + }
> > >
> > > I don't know that it makes sense to check after _every_ page. I might
> > > go for every segment. Some users check after every thousand pages.
> > >
> >
> > The handful of examples I come across on a brief scan (including the
> > other iomap usage) have a similar pattern as used here. I don't doubt
> > there are others, but I think I'd prefer to have more reasoning behind
> > adding more code than might be necessary (i.e. do we expect additional
> > overhead to be measurable here?). As it is, the intent isn't so much to
> > check on every page as much as this just happens to be the common point
> > of the function to cover both long bio chains and single vector bios
> > with large numbers of pages.
>
> It's been a while since I waded through the macro hell to find out what
> cond_resched actually does, but iirc it can do some fairly heavyweight
> things (disable preemption, call the scheduler, rcu stuff) which is why
> we're supposed to be a little judicious about amortizing each call over
> a few thousand pages.
>
It looks to me it just checks some state bit and only does any work if
actually necessary. I suppose not doing that less often is cheaper than
doing it more, but it's not clear to me it's enough that it really
matters and/or warrants more code to filter out calls..
What exactly did you have in mind for logic? I suppose we could always
stuff a 'if (!(count++ % 1024)) cond_resched();' or some such in the
inner loop, but that might have less of an effect on larger chains
constructed of bios with fewer pages (depending on whether that might
still be possible).
Brian
> --D
>
> > Brian
> >
>
