On Mon, Jan 30, 2023 at 04:12:25PM +0500, Muhammad Usama Anjum wrote:
[...]
> >
> >> +#define IS_WP_ENGAGE_OP(a) (a->flags & PAGEMAP_WP_ENGAGE)
> >> +#define IS_GET_OP(a) (a->vec)
> >
> > Having a->vec to imply the GET is fine, but IMHO not as clean as having
> > each OP a bit in the flags.
> >
> > How about:
> >
> > #define PAGEMAP_OP_GET (1UL << 0)
> > #define PAGEMAP_OP_WP (1UL << 1)
> > #define PAGEMAP_OP_MASK (PAGEMAP_OP_GET | PAGEMAP_OP_WP)
> >
> > ?
> >
> > Then a->vec requried for PAGEMAP_OP_GET.
> I had something like PAGEMAP_OP_GET and PAGEMAP_OP_WP flags once until I
> was asked to only keep WP flag and vec != NULL will imply GET flag in a
> previous review. I'm ready to change again if there are more notes on it?
Sorry to know that; that's somewhat frustrating when you need to go back
and forth on subjective comments like mine. So if you still think your
original way is better then at least you have two votes now. :) Your call
to choose any, I have a preference as I said but not that strong.
[...]
> >
> >> + struct pagemap_scan_private *p, unsigned long addr, unsigned int len)
> >> +{
> >> + unsigned long bitmap, cur = PAGEMAP_SCAN_BITMAP(wt, file, pres, swap);
> >> + bool cpy = true;
> >> + struct page_region *prev = &p->prev;
> >> +
> >> + if (HAS_NO_SPACE(p))
> >> + return -ENOSPC;
> >
> > This can be moved to below [1], we should stop scanning immediately if the
> > condition met.
> It would be definately possible. Wouldn't it be strange to return error
> when the operation was successful? A function should return error only when
> the current execution is unsuccessful. So I'm returning error only when the
> space is full and there is no space left in the buffer anymore.
I would expect the user to always provide some more space than they expect
because the merging of page_regions are kind of unpredictable from the
user's POV.
But yeah, maybe you're right. I'm fine to keep that as is.
Thanks,
--
Peter Xu
