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