Re: [RFC PATCH v2 3/5] mm: Make swap_readpage() for SWP_FS_OPS use ->direct_IO() not ->readpage()

Christoph Hellwig
2021-08-13T07:12:57Z

> +/*
> + * Keep track of the kiocb we're using to do async DIO. We have to
> + * refcount it until various things stop looking at the kiocb *after*
> + * calling ->ki_complete().
> + */
> +struct swapfile_kiocb {
> + struct kiocb iocb;
> + refcount_t ki_refcnt;
> +};

The ki_ prefix is a little strange here.

> +
> +static void swapfile_put_kiocb(struct swapfile_kiocb *ki)
> +{
> + if (refcount_dec_and_test(&ki->ki_refcnt)) {
> + fput(ki->iocb.ki_filp);

What do we need the file reference for here? The swap code has to have
higher level prevention for closing the file vs active I/O, at least the
block path seems to rely on that.

> +static void swapfile_read_complete(struct kiocb *iocb, long ret, long ret2)
> +{
> + struct swapfile_kiocb *ki = container_of(iocb, struct swapfile_kiocb, iocb);

Overly long line.

> + /* Should set IOCB_HIPRI too, but the box becomes unresponsive whilst
> + * putting out occasional messages about the NFS sunrpc scheduling
> + * tasks being hung.
> + */

IOCB_HIPRI has a very specific meaning, so I'm not sure we should
use it never mind leave such a comment here. Also this is not the
proper standard kernel comment style.

> +
> + iov_iter_bvec(&to, READ, &bv, 1, thp_size(page));
> + ret = swap_file->f_mapping->a_ops->direct_IO(&kiocb, &to);
> +
> + __swapfile_read_complete(&kiocb, ret, 0);
> + return (ret > 0) ? 0 : ret;

No need for the braces.

> + return (ret > 0) ? 0 : ret;

Same here.