From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0FA83C433EF for ; Sat, 11 Jun 2022 05:23:51 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 08DD26B017D; Sat, 11 Jun 2022 01:23:51 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 016446B017E; Sat, 11 Jun 2022 01:23:50 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DD1CE8D00F7; Sat, 11 Jun 2022 01:23:50 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id C7D1D6B017D for ; Sat, 11 Jun 2022 01:23:50 -0400 (EDT) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay11.hostedemail.com (Postfix) with ESMTP id 96CBF81545 for ; Sat, 11 Jun 2022 05:23:50 +0000 (UTC) X-FDA: 79564813020.26.76FB8CA Received: from p3plwbeout27-01.prod.phx3.secureserver.net (p3plsmtp27-01-2.prod.phx3.secureserver.net [216.69.139.46]) by imf17.hostedemail.com (Postfix) with ESMTP id BA2A540058 for ; Sat, 11 Jun 2022 05:23:49 +0000 (UTC) Received: from mailex.mailcore.me ([94.136.40.142]) by :WBEOUT: with ESMTP id ztb9nVBE3zvXJztbAn2Nns; Fri, 10 Jun 2022 22:23:48 -0700 X-CMAE-Analysis: v=2.4 cv=fbZod2cF c=1 sm=1 tr=0 ts=62a426e4 a=s1hRAmXuQnGNrIj+3lWWVA==:117 a=84ok6UeoqCVsigPHarzEiQ==:17 a=ggZhUymU-5wA:10 a=IkcTkHD0fZMA:10 a=JPEYwPQDsx4A:10 a=VwQbUJbxAAAA:8 a=FXvPX3liAAAA:8 a=9JzQIcLJ2qe6kJ3kX6oA:9 a=QEXdDO2ut3YA:10 a=AjGcO6oz07-iQ99wixmX:22 a=UObqyxdv-6Yh2QiB9mM_:22 X-SECURESERVER-ACCT: phillip@squashfs.org.uk X-SID: ztb9nVBE3zvXJ Received: from 82-69-79-175.dsl.in-addr.zen.co.uk ([82.69.79.175] helo=[192.168.178.33]) by smtp13.mailcore.me with esmtpa (Exim 4.94.2) (envelope-from ) id 1nztb8-0001Si-QI; Sat, 11 Jun 2022 06:23:47 +0100 Message-ID: Date: Sat, 11 Jun 2022 06:23:42 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.10.0 Subject: Re: [PATCH v5 3/3] squashfs: implement readahead To: Hsin-Yi Wang , Matthew Wilcox , Xiongwei Song , Marek Szyprowski , Andrew Morton Cc: Zheng Liang , Zhang Yi , Hou Tao , Miao Xie , "linux-mm @ kvack . org" , "squashfs-devel @ lists . sourceforge . net" , linux-kernel@vger.kernel.org References: <20220606150305.1883410-1-hsinyi@chromium.org> <20220606150305.1883410-4-hsinyi@chromium.org> From: Phillip Lougher In-Reply-To: <20220606150305.1883410-4-hsinyi@chromium.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Mailcore-Auth: 439999529 X-Mailcore-Domain: 1394945 X-123-reg-Authenticated: phillip@squashfs.org.uk X-Originating-IP: 82.69.79.175 X-CMAE-Envelope: MS4xfJjc6MXG/CoqLMpZuyp3VfiyYHDy7QkcyMdJl74xc03IOOrWN1uHEBNm8SjaNnbzHV3ihJ3DImYn81rYQkZly+UTfMxYdQsDvilQRxfV/ScgRIBAZekS c/p4ybCrKG2k+iJYeuCWxx77mxHCurpmXrS+qxJ6OR45x9JJfbTBw9OFggsf0WwcDn7vkxbPXAbwMZMmaqGC93e/Ft9BViIkmlY= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1654925030; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=iBlplB3iWTffq+yqtPO+GO5mYtl+OM5cofSlR1oHORY=; b=Mvc9A1oDHoB+vsJNL/UozkbUQ0GwSlXKd8Kf5rJfKnwyk3y3uWOnnHjO27MGl/HEzqXg08 o8vikPW0kNPcKyNsY+jPfgXtSkr/IDOQHFUmnKw9NjVTYWT2Y0/j3IYs1eMx0y3I4IjUmi yX6FxSk5D8esXm/ZM0o8SbIJE8LeLb0= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1654925030; a=rsa-sha256; cv=none; b=d6pthkIFMEJcNebboQIOq8VQpRSXPV3okAC3oFr21vh4//ex88kdqbDBJuqYiOkl32zIW0 bk//h2G61GG7OfMCji6f+NoGPEIgsyoq9I62uHvkCNj03p3Jtag9feP9lDCVJT4R24kzgR DlZIkpG/mLaouvDpgg5dgzGKsVzd7Oo= ARC-Authentication-Results: i=1; imf17.hostedemail.com; dkim=none; dmarc=none; spf=none (imf17.hostedemail.com: domain of phillip@squashfs.org.uk has no SPF policy when checking 216.69.139.46) smtp.mailfrom=phillip@squashfs.org.uk X-Rspam-User: Authentication-Results: imf17.hostedemail.com; dkim=none; dmarc=none; spf=none (imf17.hostedemail.com: domain of phillip@squashfs.org.uk has no SPF policy when checking 216.69.139.46) smtp.mailfrom=phillip@squashfs.org.uk X-Rspamd-Server: rspam03 X-Stat-Signature: wfaa1y3qgbiuohcxygewxs33doe9aczw X-Rspamd-Queue-Id: BA2A540058 X-HE-Tag: 1654925029-242667 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 06/06/2022 16:03, Hsin-Yi Wang wrote: > Implement readahead callback for squashfs. It will read datablocks > which cover pages in readahead request. For a few cases it will > not mark page as uptodate, including: > - file end is 0. > - zero filled blocks. > - current batch of pages isn't in the same datablock. > - decompressor error. > Otherwise pages will be marked as uptodate. The unhandled pages will be > updated by readpage later. > Hi Hsin-Yi, I have reviewed, tested and instrumented the following patch. There are a number of problems with the patch including performance, unhandled issues, and bugs. In this email I'll concentrate on the performance aspects. The major change between this V5 patch and the previous patches (V4 etc), is that it now handles the case where + nr_pages = __readahead_batch(ractl, pages, max_pages); returns an "nr_pages" less than "max_pages". What this means is that the readahead code has returned a set of page cache pages which does not fully map the datablock to be decompressed. If this is passed to squashfs_read_data() using the current "page actor" code, the decompression will fail on the missing pages. In recognition of that fact, your V5 patch falls back to using the earlier intermediate buffer method, with squashfs_get_datablock() returning a buffer, which is then memcopied into the page cache pages. This is currently what is also done in the existing squashfs_readpage_block() function if the entire set of pages cannot be obtained. The problem with this fallback intermediate buffer is it is slow, both due to the additional memcopies, but, more importantly because it introduces contention on a single shared buffer. I have long had the intention to fix this performance issue in squashfs_readpage_block(), but, due it being a rare issue there, the additional work has seemed to be nice but not essential. The problem is we don't want the readahead code to be using this slow method, because the scenario will probably happen much more often, and for a performance improvement patch, falling back to an old slow method isn't very useful. So I have finally done the work to make the "page actor" code handle missing pages. This I have sent out in the following patch-set updating the squashfs_readpage_block() function to use it. https://lore.kernel.org/lkml/20220611032133.5743-1-phillip@squashfs.org.uk/ You can use this updated "page actor" code to eliminate the "nr_pages < max_pages" special case in your patch. With the benefit that decompression is done directly into the page cache. I have updated your patch to use the new functionality. The diff including a bug fix I have appended to this email. Phillip diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c index b86b2f9d9ae6..721d35ecfca9 100644 --- a/fs/squashfs/file.c +++ b/fs/squashfs/file.c @@ -519,10 +519,6 @@ static void squashfs_readahead(struct readahead_control *ractl) if (!pages) return; - actor = squashfs_page_actor_init_special(pages, max_pages, 0); - if (!actor) - goto out; - for (;;) { pgoff_t index; int res, bsize; @@ -548,41 +544,21 @@ static void squashfs_readahead(struct readahead_control *ractl) if (bsize == 0) goto skip_pages; - if (nr_pages < max_pages) { - struct squashfs_cache_entry *buffer; - unsigned int block_mask = max_pages - 1; - int offset = pages[0]->index - (pages[0]->index & ~block_mask); - - buffer = squashfs_get_datablock(inode->i_sb, block, - bsize); - if (buffer->error) { - squashfs_cache_put(buffer); - goto skip_pages; - } - - expected -= offset * PAGE_SIZE; - for (i = 0; i < nr_pages && expected > 0; i++, - expected -= PAGE_SIZE, offset++) { - int avail = min_t(int, expected, PAGE_SIZE); - - squashfs_fill_page(pages[i], buffer, - offset * PAGE_SIZE, avail); - unlock_page(pages[i]); - } - - squashfs_cache_put(buffer); - continue; - } + actor = squashfs_page_actor_init_special(msblk, pages, nr_pages, expected); + if (!actor) + goto out; res = squashfs_read_data(inode->i_sb, block, bsize, NULL, actor); + kfree(actor); + if (res == expected) { int bytes; - /* Last page may have trailing bytes not filled */ + /* Last page (if present) may have trailing bytes not filled */ bytes = res % PAGE_SIZE; - if (bytes) { + if (pages[nr_pages - 1]->index == file_end && bytes) { void *pageaddr; pageaddr = kmap_atomic(pages[nr_pages - 1]); @@ -602,7 +578,6 @@ static void squashfs_readahead(struct readahead_control *ractl) } } - kfree(actor); kfree(pages); return; @@ -612,7 +587,6 @@ static void squashfs_readahead(struct readahead_control *ractl) put_page(pages[i]); } - kfree(actor); out: kfree(pages); } -- 2.34.1