From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from verein.lst.de (verein.lst.de [213.95.11.211]) (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 CD62A37F313 for ; Tue, 28 Jul 2026 04:17:04 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=213.95.11.211 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785212228; cv=none; b=GvAGC5bX10Yx5le1HYpBDaAg9LZnm9P+F3RkrQ59/6HMWAomMz6h+erce9knpsCCYDiGEtmIQ+3fAO63EB1Ig0QylBsWIS4c5HY4olpHcJaNf07ZBrsnMQRnKvvjgejVb38HlC4FDI0pDwSoIta33Ph4d7RDzkmD1qbK4gT0O4Q= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785212228; c=relaxed/simple; bh=eDQRG2ugfLw1csQ6DocqOaravUSuWyXZWR3fdxS9HRA=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Ke1FodpYJA2OdAPZv2pNQVmDnU7HO8GVvXqM+Ak2xtbVwV4D1cSrm2NKbN4uEh6Ex3hIo4bp3AkAZqOf/ICxMKmmDg7NFaTE2/wUWoxElpX8/tyq4JlTadm0anxy1yE1VoKLkUs6R9hwjtXHmPUjuk5f3c1KAj8YtBOWZUR4WgI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=lst.de; spf=pass smtp.mailfrom=lst.de; arc=none smtp.client-ip=213.95.11.211 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=lst.de Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=lst.de Received: by verein.lst.de (Postfix, from userid 2407) id 70FF268AFE; Tue, 28 Jul 2026 06:16:57 +0200 (CEST) Date: Tue, 28 Jul 2026 06:16:57 +0200 From: Christoph Hellwig To: Heming Zhao Cc: joseph.qi@linux.alibaba.com, mark@fasheh.com, jlbec@evilplan.org, hch@lst.de, ocfs2-devel@lists.linux.dev, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH v2 2/4] ocfs2: switch dio read path from buffer_head to iomap Message-ID: <20260728041657.GB19573@lst.de> References: <20260727061802.18485-1-heming.zhao@suse.com> <20260727061802.18485-3-heming.zhao@suse.com> 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: <20260727061802.18485-3-heming.zhao@suse.com> User-Agent: Mutt/1.5.17 (2007-11-01) On Mon, Jul 27, 2026 at 02:17:58PM +0800, Heming Zhao wrote: > This patch migrates the DIO read path from the legacy buffer_head Please never start commit messages with "This patch", that context is implied already. > The rw cluster-lock level is not stashed in iocb->private: iomap DIO owns Explaining what you don't do is always a bit odd, I'd start with what you do, and only really mention what not later if really needed. > that field (it stores a bio there for polled I/O and unconditionally > clears it before calling ->end_io), so any side-channel packed into > iocb->private is corrupted and would trip the end_io lock-state check. > Instead the unlock level is encoded by which iomap_dio_ops is passed - > ocfs2_iomap_dio_ops_pr (PRMODE) or ocfs2_iomap_dio_ops_ex (EXMODE) - and > the *_iter() functions use the split __iomap_dio_rw() + iomap_dio_complete() > API so they can determine unambiguously, from the return value, whether the > completion handler already dropped the lock (real I/O, sync or -EIOCBQUEUED) > or whether the caller must drop it (no I/O issued, or buffered fallback). .. and a lot of this just seems to be about the locking, but very little about the iomap changes. > + if (flags & IOMAP_WRITE) { > + /* todo */ should this be a WARN_ON for now? > + if (direct_io && ocfs2_should_use_dio(iocb, to, inode)) { > + struct iomap_dio *dio; > + > + dio = __iomap_dio_rw(iocb, to, &ocfs2_iomap_ops, > + &ocfs2_iomap_dio_ops_r_pr, 0, NULL, 0); > + if (dio == NULL) { > + /* No I/O issued; rw_lock still held. */ > + ret = 0; > + } else if (IS_ERR(dio)) { > + ret = PTR_ERR(dio); > + if (ret == -EIOCBQUEUED) > + rw_level = -1; > + } else { > + ret = iomap_dio_complete(dio); > + if (ret != 0) > + rw_level = -1; > + } What information is missing if you use plain iomap_dio_rw here? > + /* > + * A 0 result means the mapping bounced us back to buffered I/O > + * (e.g. inline data); the rw_lock is still held. Clear > + * IOCB_DIRECT so generic_file_read_iter() takes the buffered > + * path rather than re-entering direct I/O. > + */ > + if (ret == 0) { > + iocb->ki_flags &= ~IOCB_DIRECT; > + ret = generic_file_read_iter(iocb, to); > + } > + } else { > + iocb->ki_flags &= ~IOCB_DIRECT; > + ret = generic_file_read_iter(iocb, to); > + } Can you share this code somehow?