Re: [PATCH 03/31] fs: introduce new ->get_poll_head and ->poll_mask methods

[Al Viro]

On Mon, Jan 08, 2018 at 11:45:13AM +0100, Christoph Hellwig wrote:
> On Sat, Jan 06, 2018 at 07:12:42PM +0000, Al Viro wrote:
> > On Thu, Jan 04, 2018 at 09:00:15AM +0100, Christoph Hellwig wrote:
> > > ->get_poll_head returns the waitqueue that the poll operation is going
> > > to sleep on.  Note that this means we can only use a single waitqueue
> > > for the poll, unlike some current drivers that use two waitqueues for
> > > different events.  But now that we have keyed wakeups and heavily use
> > > those for poll there aren't that many good reason left to keep the
> > > multiple waitqueues, and if there are any ->poll is still around, the
> > > driver just won't support aio poll.
> > 
> > *UGH*
> > 
> > Gotta love the optimism, but have you actually done the conversion?
> > I'm particularly suspicious about the locking rules here...
> 
> I've done just about everything but random drivers.  Which is the ones
> where people care about performance and thus aio poll.  I suspect that
> we will have various odd cruft drivers that will be left alone.

*snort*

Seeing that random drivers are, by far, the majority of instances...
What I wonder is how many of them conform to that pattern and how
many can be massaged to that form.

How painful would it be, to pick an instance with more than one wait
queue involved, to convert drivers/char/random.c to that form?

FWIW, I agree that it's very common.  Which makes the departures from
that pattern worth looking into - they might be buggy.  And "more than
one queue to wait on" is not all - there's also e.g.
static unsigned int vtpm_proxy_fops_poll(struct file *filp, poll_table *wait)
{
        struct proxy_dev *proxy_dev = filp->private_data;
        unsigned ret;

        poll_wait(filp, &proxy_dev->wq, wait);

        ret = POLLOUT;

        mutex_lock(&proxy_dev->buf_lock);

        if (proxy_dev->req_len)
                ret |= POLLIN | POLLRDNORM;

        if (!(proxy_dev->state & STATE_OPENED_FLAG))
                ret |= POLLHUP;

        mutex_unlock(&proxy_dev->buf_lock);

        return ret;
} 
(mainline drivers/char/tpm/tpm_vtpm_proxy.c)

Is that mutex_lock() in there a bug?  Another fun case is dma_buf_poll()...

The reason why I went looking at ->poll() in the first place had been
recurring bugs in the instances; e.g. "oh, I've got something odd,
let's return -Esomething".  Or "was it POLLIN or POLL_IN?"

I'd really like to get the interfaces right and get rid of the bitrot
source; turning it into moldering corpse in the corner is fine, as
long as we have a realistic chance of getting rid of that body in not
too distant future...