RE: [PATCH v2 02/10] fsdax: Factor helper: dax_fault_actor()

[ruansy.fnst@xxxxxxxxxxx]

> > +	/* 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!