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 C3FE1C433EF for ; Wed, 1 Jun 2022 08:37:08 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1AA186B008A; Wed, 1 Jun 2022 04:37:08 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 132786B008C; Wed, 1 Jun 2022 04:37:08 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id F39396B0092; Wed, 1 Jun 2022 04:37:07 -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 E230B6B008A for ; Wed, 1 Jun 2022 04:37:07 -0400 (EDT) Received: from smtpin31.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id A010360A27 for ; Wed, 1 Jun 2022 08:37:07 +0000 (UTC) X-FDA: 79529012094.31.7EDBE14 Received: from mail-ed1-f48.google.com (mail-ed1-f48.google.com [209.85.208.48]) by imf27.hostedemail.com (Postfix) with ESMTP id EBE944005D for ; Wed, 1 Jun 2022 08:37:02 +0000 (UTC) Received: by mail-ed1-f48.google.com with SMTP id x62so1154011ede.10 for ; Wed, 01 Jun 2022 01:37:06 -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=XZXVqu+h9NXbHvrC2CE+JHc/yJMidzZ16AsBJPRrJPE=; b=V0iPmt+kTBJH5ivwrDvWTMPC9WwYqethXf9BHUztx9gbkHKvHcvpU6Ty1cRNR4mHPd sy43RmmEcF13bNjbgYbYDMygYypovBn5LwM4kIw4hq/ucZuP7/72mTjQIg9seBTTX4iP iN7Gtk0gAfuRXlU6PqGiHGz7dupYyUlAQXeHY= 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=XZXVqu+h9NXbHvrC2CE+JHc/yJMidzZ16AsBJPRrJPE=; b=KY0tf2IIZxFTOeHYAqe4MA2QE91yfN9gXmUO2ZcMqpknsow2z9LikEtsMS9TKk1llP SPcIQ2/jp9V7JksoLD3SVHmb5jywm4RPTjOdN/21e2rwwGB0gvJ0TYMfMxcpxXrdH49B 4dZ1ExSReITHVzQ7A9XHhWPi2cldRzQsbHk0hO6KHdpprcpRoiKaPA0g8D/FCN74QGDn fGUru7Q45Kg25Cz86VsiUzittKEt+UkCEEGdKL50awBQaaV+7xRDIuVfmfwQhVXq+R1+ ypvSue5cRGs2CEnK2E71hlXea5XyonGmzGzTM8B8keMt23XcbOMbwkBSV0X9DqP1Ai1M +vig== X-Gm-Message-State: AOAM530Wtd8yeOsd8Z11aPujCirnyJY42OaeEbqYQCvQQAa28YgeMrem o9OYQytpfW+DUDBlMiVPZkyGI2/ujTfkDdKNJ/eQFg== X-Google-Smtp-Source: ABdhPJymQSOKf855e7dfllaefJZ/lv3bCjmLAxuMnHWvIuClKGxEYglMyqZaP4fn7g0f7+FddB96W87/zPbulc5iUYc= X-Received: by 2002:a05:6402:f1b:b0:42d:e92f:c924 with SMTP id i27-20020a0564020f1b00b0042de92fc924mr1936092eda.389.1654072625416; Wed, 01 Jun 2022 01:37:05 -0700 (PDT) MIME-Version: 1.0 References: <20220523065909.883444-1-hsinyi@chromium.org> <20220523065909.883444-4-hsinyi@chromium.org> <20220531134740.91ae4dcea1e06640ba1bfc12@linux-foundation.org> In-Reply-To: From: Hsin-Yi Wang Date: Wed, 1 Jun 2022 16:36:39 +0800 Message-ID: Subject: Re: [PATCH v3 3/3] squashfs: implement readahead To: Phillip Lougher Cc: Andrew Morton , Matthew Wilcox , Xiongwei Song , Zheng Liang , Zhang Yi , Hou Tao , Miao Xie , "linux-mm @ kvack . org" , "squashfs-devel @ lists . sourceforge . net" , linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: EBE944005D X-Stat-Signature: 9zsjeczdd8qge7xtbs87u9mfqgrnenq7 X-Rspam-User: Authentication-Results: imf27.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b=V0iPmt+k; spf=pass (imf27.hostedemail.com: domain of hsinyi@chromium.org designates 209.85.208.48 as permitted sender) smtp.mailfrom=hsinyi@chromium.org; dmarc=pass (policy=none) header.from=chromium.org X-HE-Tag: 1654072622-689179 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 Wed, Jun 1, 2022 at 9:08 AM Phillip Lougher wrote: > > On 31/05/2022 21:47, Andrew Morton wrote: > > On Mon, 23 May 2022 14:59:13 +0800 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 or not enough in a > >> datablock. > >> - decompressor error. > >> Otherwise pages will be marked as uptodate. The unhandled pages will be > >> updated by readpage later. > >> > >> ... > >> > > > > The choice of types seems somewhat confused. > > > >> @@ -495,7 +496,95 @@ static int squashfs_read_folio(struct file *file, struct folio *folio) > >> return 0; > >> } > >> > >> +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; > > > > block_log is unsigned short. Why size_t? Will update in the next version. > > > >> + loff_t start = readahead_pos(ractl) &~ mask; > >> + size_t len = readahead_length(ractl) + readahead_pos(ractl) - start; > >> + struct squashfs_page_actor *actor; > >> + unsigned int nr_pages = 0; > > > > OK. > > > >> + struct page **pages; > >> + u64 block = 0; > >> + int bsize, res, i, index, bytes, expected; > > > > `res' could be local to the inner loop. > > > > `i' is used in situations where an unsigned type would be more > > appropriate. If it is made unsigned then `i' is no longer a suitable > > identifier. Doesn't matter much. > > > > `index' is from page.index, which is pgoff_t. > > > > `bytes' could be local to the innermost loop. > > > > `expected' is inappropriately a signed type and could be local to the > > inner loop. Will update them in the next version. > > > >> + int file_end = i_size_read(inode) >> msblk->block_log; > >> + unsigned int max_pages = 1UL << shift; > >> + void *pageaddr; > >> + > > pageaddr could be made local to the innermost scope. > Will update them in the next version. Thanks for your comments. > Apart from that the patch and updated error handling looks > good. > > Phillip > > >> + readahead_expand(ractl, start, (len | mask) + 1); > >> + > >> + if (file_end == 0) > >> + return; > >> + > >> + pages = kmalloc_array(max_pages, sizeof(void *), GFP_KERNEL); > >> + if (!pages) > >> + return; > >> + > >> + actor = squashfs_page_actor_init_special(pages, max_pages, 0); > >> + if (!actor) > >> + goto out; > >> + > >> + for (;;) { > >> + nr_pages = __readahead_batch(ractl, pages, max_pages); > >> + if (!nr_pages) > >> + break; > >> + > >> + if (readahead_pos(ractl) >= i_size_read(inode) || > >> + nr_pages < max_pages) > >> + goto skip_pages; > >> + > >> + index = pages[0]->index >> shift; > >> + if ((pages[nr_pages - 1]->index >> shift) != index) > >> + goto skip_pages; > >> + > >> + expected = index == file_end ? > >> + (i_size_read(inode) & (msblk->block_size - 1)) : > >> + msblk->block_size; > >> + > >> + bsize = read_blocklist(inode, index, &block); > >> + if (bsize == 0) > >> + goto skip_pages; > >> + > >> + res = squashfs_read_data(inode->i_sb, block, bsize, NULL, > >> + actor); > >> + > >> + if (res == expected) { > >> + /* Last page may have trailing bytes not filled */ > >> + bytes = res % PAGE_SIZE; > >> + if (bytes) { > >> + pageaddr = kmap_atomic(pages[nr_pages - 1]); > >> + memset(pageaddr + bytes, 0, PAGE_SIZE - bytes); > >> + kunmap_atomic(pageaddr); > >> + } > >> + > >> + for (i = 0; i < nr_pages; i++) > >> + SetPageUptodate(pages[i]); > >> + } > > > > res == -EIO is unhandled? > > > >> + for (i = 0; i < nr_pages; i++) { > >> + unlock_page(pages[i]); > >> + put_page(pages[i]); > >> + } > >> + } > >> + > >> + kfree(actor); > >> + kfree(pages); > >> + return; > >> + > >> +skip_pages: > >> + for (i = 0; i < nr_pages; i++) { > >> + unlock_page(pages[i]); > >> + put_page(pages[i]); > >> + } > >> + > >> + kfree(actor); > >> +out: > >> + kfree(pages); > >> +} > >> > >> const struct address_space_operations squashfs_aops = { > >> - .read_folio = squashfs_read_folio > >> + .read_folio = squashfs_read_folio, > >> + .readahead = squashfs_readahead > >> }; > > >