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 7D4DEC433F5 for ; Fri, 13 May 2022 06:35:43 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1ACB46B0075; Fri, 13 May 2022 02:35:43 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 15D168D0002; Fri, 13 May 2022 02:35:43 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 04C608D0001; Fri, 13 May 2022 02:35:42 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id EAB196B0075 for ; Fri, 13 May 2022 02:35:42 -0400 (EDT) Received: from smtpin27.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id C00A031D54 for ; Fri, 13 May 2022 06:35:42 +0000 (UTC) X-FDA: 79459758924.27.AE05778 Received: from mail-vk1-f181.google.com (mail-vk1-f181.google.com [209.85.221.181]) by imf12.hostedemail.com (Postfix) with ESMTP id 4576E4009E for ; Fri, 13 May 2022 06:35:18 +0000 (UTC) Received: by mail-vk1-f181.google.com with SMTP id o132so3733678vko.11 for ; Thu, 12 May 2022 23:35:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=VhFUDd1DqlQKUTmhLS6hQUgklWrN9WXEYPO4jAOfCwE=; b=IjFV6dj3r23Q9t0eTX7d7jmaSSDUJHBCOCcTHNz/G1ttzsSBI4pAcAl4TAIyax4rf7 Io3oa5HC/M0N9yizqW5a8NebKkcQ3atJiakVB6wcQ5HE740iSheStuxZIApax5hGR6Hi u2aO+4TUlqtylJQAzTdVnxSsfo7rmrMdBNTL8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=VhFUDd1DqlQKUTmhLS6hQUgklWrN9WXEYPO4jAOfCwE=; b=hJ4lW6BgJGPLtmMu/vMQK0ChD0WfKk6Zt/PvGc07GRDP93j4bSWPAejlpsLXZb2tZ0 7PyCx4IYHbcM0Vl/usLnELul4PcxR5l/YnV1fQY04sYGoNl5wfv3jbL3G3sdGR4GGoKX Q1KVVPkbqqpza+GySCPqew+DXcz/4MyfRgH3eRnEfup0Qzs8qOGi/wBDGf6ouLoa0Z3P yoO4c0mZe9AGH/rpawRRFkIyk8FVJp+pH8tmuCXjdpRRYpsj3jxKFqwpdpubyG/duq4E BdW9tzR/KUCM4/iCbgFMQvh2K4ctEgitH1OyLejCqxX06YuXidoHyCSswSY7lWqQ6lWg IB0A== X-Gm-Message-State: AOAM532Ko2stavZUUgWmjauaIFuSbmjsJ4GeX/Gsdpz7JlJPs1jFN25i DqhbJA+ufEakSMMg0sKyooQ3EP6euTV2d37IH/RGXQ== X-Google-Smtp-Source: ABdhPJxLZaeW4s2OqCBrEOyQPurUpHZigCAWnINSzL5ZnuAfYlhqug56+a8bFTxwlaM/l4AbjHUtVMbSq3npCJZ8Xys= X-Received: by 2002:a05:6122:da8:b0:331:3b30:8b40 with SMTP id bc40-20020a0561220da800b003313b308b40mr1686055vkb.30.1652423741507; Thu, 12 May 2022 23:35:41 -0700 (PDT) MIME-Version: 1.0 References: <1f8a8009-1c05-d55c-08bd-89c5916e5240@squashfs.org.uk> In-Reply-To: <1f8a8009-1c05-d55c-08bd-89c5916e5240@squashfs.org.uk> From: Hsin-Yi Wang Date: Fri, 13 May 2022 14:35:14 +0800 Message-ID: Subject: Re: squashfs performance regression and readahea To: Phillip Lougher Cc: Xiongwei Song , Matthew Wilcox , Zheng Liang , Zhang Yi , Hou Tao , Miao Xie , Andrew Morton , Linus Torvalds , "Song, Xiongwei" , "linux-mm@kvack.org" , "squashfs-devel@lists.sourceforge.net" Content-Type: text/plain; charset="UTF-8" X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: 4576E4009E Authentication-Results: imf12.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b=IjFV6dj3; spf=pass (imf12.hostedemail.com: domain of hsinyi@chromium.org designates 209.85.221.181 as permitted sender) smtp.mailfrom=hsinyi@chromium.org; dmarc=pass (policy=none) header.from=chromium.org X-Rspam-User: X-Stat-Signature: jnirtbz7rn8uuqg6cqamub68thceagec X-HE-Tag: 1652423718-311029 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 Fri, May 13, 2022 at 1:33 PM Phillip Lougher wrote: > > On 12/05/2022 07:23, Hsin-Yi Wang wrote: > > > > > Hi Matthew, > > Thanks for reviewing the patch previously. Does this version look good > > to you? If so, I can send it to the list. > > > > > > Thanks for all of your help. > > Hi Hsin-Yi Wang, > Hi Philip, > Thanks for updating the patch to minimise the pages used when > creating the page actor. > > Looking at the new patch, I have a couple of questions which is worth > clarifying to have a fuller understanding of the readahead behaviour. > In otherwords I'm deducing the behaviour of the readahead calls > from context, and I want to make sure they're doing what I think > they're doing. > > + nr_pages = min(readahead_count(ractl), max_pages); > > As I understand it, this will always produce nr_pages which will > cover the entirety of the block to be decompressed? That is if > a block is block_size, it will return the number of pages necessary > to decompress the entire block? It will never return less than the > necessary pages, i.e. if the block_size was 128K, it will never > return less than 32 pages? > In most of the cases the size is max_pages (readahead_count(ractl) == max_pages). > Similarly, if at the end of the file, where the last block may not > be a full block (i.e. less than block_size) it will only return > the pages covered by the tail end block, not a full block. For > example, if the last block is 16 Kbytes (and block_size is > 128 Kbytes), it will return 4 pages and not 32 pages ... > But it's possible that readahead_count(ractl) < max_pages at the end of file. > Obviously this behaviour is important for decompression, because > you must always have enough pages to decompress the entire block > into. > > You also shouldn't pass in more pages than expected (i.e. if the > block is only expected to decompress to 4 pages, that's what you > pass, rather than the full block size). This helps to trap > corrupted blocks, where they are prevented to decompress to larger > than expected. > > + nr_pages = __readahead_batch(ractl, pages, max_pages) > > My understanding is that this call will fully populate the > pages array with page references without any holes. That > is none of the pages array entries will be NULL, meaning > there isn't a page for that entry. In other words, if the > pages array has 32 pages, each of the 32 entries will > reference a page. > I noticed that if nr_pages < max_pages, calling read_blocklist() will have SQUASHFS errors, SQUASHFS error: Failed to read block 0x125ef7d: -5 SQUASHFS error: zlib decompression failed, data probably corrupt so I did a check if nr_pages < max_pages before squashfs_read_data(), just skip the remaining pages and let them be handled by readpage. > This is important for the decompression code, because it > expects each pages array entry to reference a page, which > can be kmapped to an address. If an entry in the pages > array is NULL, this will break. > > If the pages array can have holes (NULL pointers), I have > written an update patch which allows the decompression code > to handle these NULL pointers. > > If the pages array can have NULL pointers, I can send you > the patch which will deal with this. Sure, if there are better ways to deal with this. Thanks. > > Thanks > > Phillip > > > > > > >>> > >>> It's also noticed that when the crash happened, nr_pages obtained by > >>> readahead_count() is 512. > >>> nr_pages = readahead_count(ractl); // this line > >>> > >>> 2) Normal cases that won't crash: > >>> [ 22.651750] Block @ 0xb3bbca6, compressed size 42172, src size 262144 > >>> [ 22.653580] Block @ 0xb3c6162, compressed size 29815, src size 262144 > >>> [ 22.656692] Block @ 0xb4a293f, compressed size 17484, src size 131072 > >>> [ 22.666099] Block @ 0xb593881, compressed size 39742, src size 262144 > >>> [ 22.668699] Block @ 0xb59d3bf, compressed size 37841, src size 262144 > >>> [ 22.695739] Block @ 0x13698673, compressed size 65907, src size 131072 > >>> [ 22.698619] Block @ 0x136a87e6, compressed size 3155, src size 131072 > >>> [ 22.703400] Block @ 0xb1babe8, compressed size 99391, src size 131072 > >>> [ 22.706288] Block @ 0x1514abc6, compressed size 4627, src size 131072 > >>> > >>> nr_pages are observed to be 32, 64, 256... These won't cause a crash. > >>> Other values (max_pages, bsize, block...) looks normal > >>> > >>> I'm not sure why the crash happened, but I tried to modify the mask > >>> for a bit. After modifying the mask value to below, the crash is gone > >>> (nr_pages are <=256). > >>> Based on my testing on a 300K pack file, there's no performance change. > >>> > >>> diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c > >>> index 20ec48cf97c5..f6d9b6f88ed9 100644 > >>> --- a/fs/squashfs/file.c > >>> +++ b/fs/squashfs/file.c > >>> @@ -499,8 +499,8 @@ static void squashfs_readahead(struct > >>> readahead_control *ractl) > >>> { > >>> struct inode *inode = ractl->mapping->host; > >>> struct squashfs_sb_info *msblk = inode->i_sb->s_fs_info; > >>> - size_t mask = (1UL << msblk->block_log) - 1; > >>> size_t shift = msblk->block_log - PAGE_SHIFT; > >>> + size_t mask = (1UL << shift) - 1; > >>> > >>> > >>> Any pointers are appreciated. Thanks! > >> >