On Wed, Jun 01 2022 at 5:43P -0400,
Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, Jun 1, 2022 at 1:59 PM Mike Snitzer <snitzer@xxxxxxxxxx> wrote:
> >
> > ----------------------------------------------------------------
> > - Fix DM core's dm_table_supports_poll to return false if no data
> > devices.
>
> So looking at that one (mainly because of the incomprehensible
> explanation), I do note:
>
> (a) the caller does
>
> for (i = 0; i < t->num_targets; i++) {
> ti = t->targets + i;
>
> while the callee does
>
> unsigned i = 0;
>
> while (i < dm_table_get_num_targets(t)) {
> ti = dm_table_get_target(t, i++);
>
> Now, those things are entirely equivalent, but that latter form is
> likely to generate horribly bad code because those helper functions
> aren't some kind of trivial inline, they are actually normal functions
> that are defined later in that same source file.
Yes, that needs fixing.. but not urgently so.
> Maybe a compiler will do optimizations within that source file even
> for functions that haven't been defined yet. Traditionally not.
>
> Whatever. Probably not a case where anybody cares about performance,
> but it does strike me that the "use abstractions" version probably not
> only generates worse code, it seems less legible too.
>
> Very odd pattern.
OK, I can just nuke those wrappers. But yeah, none of this setup code
is fast path.
> (b) The commit message (which is why I started looking at this) says
> that it used to return true even if there are no data devices.
>
> But dm_table_supports_poll() actually _still_ returns true for at
> least one case of no data devices: if the dm_table has no targets at
> all.
Right, I'm aware.. ugly but not a case that really matters (more below).
> So I don't know. Maybe that is a "can't happen". But since I looked at
> this, I thought I'd just point out the two oddities I found while
> doing so.
It can happen that someone loads a table without any targets but it
isn't a case that matters given IO cannot be sent anywhere. For that
to happen the DM table will have been reloaded to have targets (at
which point all will be setup properly, assuming no bugs like was
fixed here).
I do see you've since pulled the changes.
Thanks,
Mike
