All of lore.kernel.org
 help / color / mirror / Atom feed
From: Al Viro <viro@zeniv.linux.org.uk>
To: "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
Cc: Evgeniy Dushistov <dushistov@mail.ru>,
	Christian Brauner <brauner@kernel.org>,
	linux-kernel@vger.kernel.org, bpf@vger.kernel.org,
	"Venkataramanan, Anirudh" <anirudh.venkataramanan@intel.com>,
	Ira Weiny <ira.weiny@intel.com>
Subject: Re: [RESEND PATCH v4] fs/ufs: Replace kmap() with kmap_local_page()
Date: Fri, 25 Nov 2022 22:50:56 +0000	[thread overview]
Message-ID: <Y4FG0O7VWTTng5yh@ZenIV> (raw)
In-Reply-To: <Y4E++JERgUMoqfjG@ZenIV>

On Fri, Nov 25, 2022 at 10:17:28PM +0000, Al Viro wrote:

> The bottom line:
> 	* teach your ufs_put_page() to accept any address within the page
> 	* flip the ways you return page and address in ufs_get_page()
> 	* use offset_in_page(addr) instead of these addr - page_address(page)
> and you'll get a much smaller patch, with a lot less noise in it.
> What's more, offset_in_page() part can be carved out into a separate
> commit - it's valid on its own, and it makes both halves easier to
> follow.
> 
> AFAICS, similar observations apply in your sysvfs patch; the point about
> calling conventions for ufs_get_page() definitely applies there, and
> stronger than for ufs - those casts are eye-watering...

As the matter of fact, I'd probably split it into 3 steps:
	1) switch the calling conventions of ufs_get_page() - callers follow
it with something like kaddr = page_address(page); and that change makes
sense on its own.  Isolated and easy to follow.
	2) offset_in_page() changes.  Again, separate, stands on its own and
is easy to follow.
	3) kmap_local() switch itself - pass address to ufs_put_page(), etc.
Considerably smaller and less cluttered than your current variant.

  reply	other threads:[~2022-11-25 22:51 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-16 16:38 [RESEND PATCH v4] fs/ufs: Replace kmap() with kmap_local_page() Fabio M. De Francesco
2022-11-25 16:19 ` Fabio M. De Francesco
2022-11-25 22:17 ` Al Viro
2022-11-25 22:50   ` Al Viro [this message]
2022-11-26 14:12     ` Fabio M. De Francesco
2022-11-27 16:15       ` Al Viro
2022-11-28 23:06         ` Fabio M. De Francesco

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Y4FG0O7VWTTng5yh@ZenIV \
    --to=viro@zeniv.linux.org.uk \
    --cc=anirudh.venkataramanan@intel.com \
    --cc=bpf@vger.kernel.org \
    --cc=brauner@kernel.org \
    --cc=dushistov@mail.ru \
    --cc=fmdefrancesco@gmail.com \
    --cc=ira.weiny@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.