linux-kernel - Re: [RFC PATCH v2 3/5] mm: Make swap_readpage() for SWP_FS_OPS use ->direct

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210813071245.GC26339@lst.de>
Date:   Fri, 13 Aug 2021 09:12:45 +0200
From:   Christoph Hellwig <hch@....de>
To:     David Howells <dhowells@...hat.com>
Cc:     willy@...radead.org, trond.myklebust@...marydata.com,
        darrick.wong@...cle.com, hch@....de, viro@...iv.linux.org.uk,
        jlayton@...nel.org, sfrench@...ba.org,
        torvalds@...ux-foundation.org, linux-nfs@...r.kernel.org,
        linux-mm@...ck.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v2 3/5] mm: Make swap_readpage() for SWP_FS_OPS use
 ->direct_IO() not ->readpage()

> +/*
> + * 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.