linux-kernel - RE: [PATCH v2 02/10] fsdax: Factor helper: dax_fault

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <OSBPR01MB29207FE4814F0FDA5C08676AF46F9@OSBPR01MB2920.jpnprd01.prod.outlook.com>
Date:   Fri, 12 Mar 2021 09:01:15 +0000
From:   "ruansy.fnst@...itsu.com" <ruansy.fnst@...itsu.com>
To:     Christoph Hellwig <hch@....de>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-xfs@...r.kernel.org" <linux-xfs@...r.kernel.org>,
        "linux-nvdimm@...ts.01.org" <linux-nvdimm@...ts.01.org>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "darrick.wong@...cle.com" <darrick.wong@...cle.com>,
        "dan.j.williams@...el.com" <dan.j.williams@...el.com>,
        "willy@...radead.org" <willy@...radead.org>,
        "jack@...e.cz" <jack@...e.cz>,
        "viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>,
        "linux-btrfs@...r.kernel.org" <linux-btrfs@...r.kernel.org>,
        "ocfs2-devel@....oracle.com" <ocfs2-devel@....oracle.com>,
        "david@...morbit.com" <david@...morbit.com>,
        "rgoldwyn@...e.de" <rgoldwyn@...e.de>
Subject: RE: [PATCH v2 02/10] fsdax: Factor helper: dax_fault_actor()



> > +	/* if we are reading UNWRITTEN and HOLE, return a hole. */
> > +	if (!write &&
> > +	    (iomap->type == IOMAP_UNWRITTEN || iomap->type ==
> IOMAP_HOLE)) {
> > +		if (!pmd)
> > +			return dax_load_hole(xas, mapping, &entry, vmf);
> > +		else
> > +			return dax_pmd_load_hole(xas, vmf, iomap, &entry);
> > +	}
> > +
> > +	if (iomap->type != IOMAP_MAPPED) {
> > +		WARN_ON_ONCE(1);
> > +		return VM_FAULT_SIGBUS;
> > +	}
> 
> Nit: I'd use a switch statement here for a clarity:
> 
> 	switch (iomap->type) {
> 	case IOMAP_MAPPED:
> 		break;
> 	case IOMAP_UNWRITTEN:
> 	case IOMAP_HOLE:
> 		if (!write) {
> 			if (!pmd)
> 				return dax_load_hole(xas, mapping, &entry, vmf);
> 			return dax_pmd_load_hole(xas, vmf, iomap, &entry);
> 		}
> 		break;
> 	default:
> 		WARN_ON_ONCE(1);
> 		return VM_FAULT_SIGBUS;
> 	}
> 
Hi, Christoph

I did not use a switch-case here is because that I still have to introduce a 'goto' for CoW(Writing on IOMAP_UNWRITTEN and the two different iomap indicate that it is a CoW operation. Then goto IOMAP_MAPPED branch to do the data copy and pfn insertion.)  You said the 'goto' makes the code convoluted.  So, I avoided to use it and refactored this part into so much if-else, which looks similar in dax_iomap_actor().  So, what's your opinion now?


--
Thanks,
Ruan Shiyang.

> 
> > +	err = dax_iomap_pfn(iomap, pos, size, &pfn);
> > +	if (err)
> > +		goto error_fault;
> > +
> > +	entry = dax_insert_entry(xas, mapping, vmf, entry, pfn, 0,
> > +				 write && !sync);
> > +
> > +	if (sync)
> > +		return dax_fault_synchronous_pfnp(pfnp, pfn);
> > +
> > +	ret = dax_fault_insert_pfn(vmf, pfn, pmd, write);
> > +
> > +error_fault:
> > +	if (err)
> > +		ret = dax_fault_return(err);
> > +
> > +	return ret;
> 
> It seems like the only place that sets err is the dax_iomap_pfn case above.  So
> I'd move the dax_fault_return there, which then allows a direct return for
> everyone else, including the open coded version of dax_fault_insert_pfn.
> 
> I really like where this is going!