Re: [PATCH v3] ceph: fix blindly expanding the readahead windows

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, May 4, 2023 at 1:11 PM <xiubli@xxxxxxxxxx> wrote:
>
> From: Xiubo Li <xiubli@xxxxxxxxxx>
>
> Blindly expanding the readahead windows will cause unneccessary
> pagecache thrashing and also will introdue the network workload.
> We should disable expanding the windows if the readahead is disabled
> and also shouldn't expand the windows too much.
>
> Expanding forward firstly instead of expanding backward for possible
> sequential reads.
>
> Bound `rreq->len` to the actual file size to restore the previous page
> cache usage.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 49870056005c ("ceph: convert ceph_readpages to ceph_readahead")
> URL: https://lore.kernel.org/ceph-devel/20230504082510.247-1-sehuww@xxxxxxxxxxxxxxxx
> URL: https://www.spinics.net/lists/ceph-users/msg76183.html
> Cc: Hu Weiwen <sehuww@xxxxxxxxxxxxxxxx>
> Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx>
> ---
>
> V3:
> - Folded Hu Weiwen's fix and bound `rreq->len` to the actual file size.

Hi Xiubo,

This looks much better!  Just a couple of nits:

>
>
>
>  fs/ceph/addr.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index ca4dc6450887..357d9d28f202 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -188,16 +188,32 @@ static void ceph_netfs_expand_readahead(struct netfs_io_request *rreq)
>         struct inode *inode = rreq->inode;
>         struct ceph_inode_info *ci = ceph_inode(inode);
>         struct ceph_file_layout *lo = &ci->i_layout;
> +       unsigned long max_pages = inode->i_sb->s_bdi->ra_pages;
> +       unsigned long max_len = max_pages << PAGE_SHIFT;
> +       loff_t end = rreq->start + rreq->len, new_end;
>         u32 blockoff;
>         u64 blockno;
>
> -       /* Expand the start downward */
> -       blockno = div_u64_rem(rreq->start, lo->stripe_unit, &blockoff);
> -       rreq->start = blockno * lo->stripe_unit;
> -       rreq->len += blockoff;
> +       /* Readahead is disabled */
> +       if (!max_pages)
> +               return;
>
> -       /* Now, round up the length to the next block */
> -       rreq->len = roundup(rreq->len, lo->stripe_unit);
> +       /*
> +        * Try to expand the length forward by rounding  up it to the next
> +        * block, but do not exceed the file size, unless the original
> +        * request already exceeds it.
> +        */
> +       new_end = round_up(end, lo->stripe_unit);
> +       new_end = min(new_end, rreq->i_size);

This can be done on single line:

        new_end = min(round_up(end, lo->stripe_unit), rreq->i_size);

> +       if (new_end > end && new_end <= rreq->start + max_len)
> +               rreq->len = new_end - rreq->start;
> +
> +       /* Try to expand the start downward */
> +       blockno = div_u64_rem(rreq->start, lo->stripe_unit, &blockoff);
> +       if (rreq->len + blockoff <= max_len) {
> +               rreq->start = blockno * lo->stripe_unit;

Can this be written as:

                rreq->start -= blockoff;

It seems like it would be easier to read and probably cheaper to
compute too.

Thanks,

                Ilya




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux