From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-188.mta0.migadu.com (out-188.mta0.migadu.com [91.218.175.188]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id D41AC48BD42 for ; Wed, 13 May 2026 15:33:58 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=91.218.175.188 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778686441; cv=none; b=IMJgy63L3dHWXveBIeCS9FA+w2i/v1OZNDCZJfcMXgI3SRS1GAQRalyLSCa1ZpBBQA9u/OS2g0/gNFprZjcNa+U2Q2d5nbFspH4Jq8pFPy5ZojHaITpgCLVR2PDsnDxRSPXb9Z02gatZd0Ly0sY8ydk30NT1+n00spsBzBJ7IN0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778686441; c=relaxed/simple; bh=KMY/jmWa4E/0cDURyJEnK+3LOm2MrhyM6+ScYkwTN9w=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=OXO7q7dH1nbladt8thhN244XqcMKap/V5ykmBVOqvLZVvJi6FW5NNNscS2s5nDD4FACm3vRj60eq0hYYMDCBtaRXw9ewrDQGLLBGC+pnGY7fKv1qv7qyscwTWYu3mIh3wA5tdIW0o7GSwebpYtmM4zHDl1bFvEeD4jQxjjqxLI4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=uaOVvZVD; arc=none smtp.client-ip=91.218.175.188 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="uaOVvZVD" Date: Wed, 13 May 2026 23:33:50 +0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1778686436; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=d056Y3Cq6suaA/pOllX4Az81RQiajaWf0jULa062EJ8=; b=uaOVvZVD8ekjdiJoMztdeJnPg4XCOz2LitS06mcSACb15GNMk0YpJFUs7rAM6ApmoCRpDU ImYOquQ8dHNiZv+hvkUzvLHLB4SFgzBVcL0j1z1Lm9ibPALHP027rB5fLxhI/dTQJ5IVUs 2MFAP0WkuqvBWtOEJ/pGJ0loeZiFRMA= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Baoquan He To: Christoph Hellwig Cc: linux-mm@kvack.org, akpm@linux-foundation.org, chrisl@kernel.org, usama.arif@linux.dev, baohua@kernel.org, kasong@tencent.com, nphamcs@gmail.com, shikemeng@huaweicloud.com, youngjun.park@lge.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH v6 2/3] mm/swap: use swap_ops to register swap device's methods Message-ID: References: <20260512104201.716213-1-baoquan.he@linux.dev> <20260512104201.716213-3-baoquan.he@linux.dev> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Migadu-Flow: FLOW_OUT On 05/12/26 at 10:32pm, Christoph Hellwig wrote: > > }; > > > > +struct swap_ops; > > /* > > Please keep empty spaces before comments. And keeping forward ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ I may not follow, I noticed that all the comments in the code use tabs for indentation rather than spaces. > declarations at the beginning of the file makes them much easier to > organize (although the current swap.h is a complete mess not only in this > regard). Sure, will move it at the beginning of linux/swap.h. Thanks. > > > +int init_swap_ops(struct swap_info_struct *sis) > > +{ > > + /* > > + * ->flags can be updated non-atomically, but that will > > + * never affect SWP_FS_OPS, so the data_race is safe. > > + */ > > + if (data_race(sis->flags & SWP_FS_OPS)) > > + sis->ops = &bdev_fs_swap_ops; > > + /* > > + * ->flags can be updated non-atomically, but that will > > + * never affect SWP_SYNCHRONOUS_IO, so the data_race is safe. > > + */ > > + else if (data_race(sis->flags & SWP_SYNCHRONOUS_IO)) > > + sis->ops = &bdev_sync_swap_ops; > > + else > > + sis->ops = &bdev_async_swap_ops; > > + > > + if (!sis->ops || !sis->ops->read_folio || !sis->ops->write_folio) > > + return -EINVAL; > > In the long run setting these from the helpers for ->swap_activate would > make more sense. Also as ls long as the ops are a small statically > defined set these checks here are a bit odd. In fact, except of your ->swap_activate, I have my ->swap_activate too queued in my local branch. While from your comments, seems we don't have conflict because we have different concerns. Please see Youngjun's comment and my reply in this thread: https://lore.kernel.org/linux-mm/aaWiBaeSCILBCzqd@yjaykim-PowerEdge-T330/ > > I guess we just need to land this ASAP, then my swap_activate series and > then do a big round of cleanups for a coherent swap interface. This patchset is part I. I posted v1 to collect suggestions, later I took a leave, then people think the v1 can be merged firstly and helped to post v2. I later came back and continue to push this part. If you just found this patch series and come up with those cleanups, could you wait a moment until we merge this part and part II is posted? I think the split handling of block and fs in swap you mentioned is super cool, and is beyond our plan. > > > - } else if (synchronous) { > > - swap_read_folio_bdev_sync(folio, sis); > > - } else { > > - swap_read_folio_bdev_async(folio, sis); > > - } > > + sis->ops->read_folio(sis, folio, plug); > > This is a really annoying double-indirection for the SWP_FS_OPS > case. I think we should have one level of indirection here. One > way to do this would be to remove the swap_rw operation and > implement the swap_ops directly in nfs and cifs. > >