On Tue, Oct 1, 2024 at 7:14 PM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote: > > On Tue, Oct 1, 2024 at 7:06 PM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote: > > > > On Tue, Oct 1, 2024 at 7:04 PM Nhat Pham <nphamcs@xxxxxxxxx> wrote: > > > > > > On Tue, Oct 1, 2024 at 6:58 PM Nhat Pham <nphamcs@xxxxxxxxx> wrote: > > > > > > > > On Tue, Oct 1, 2024 at 6:33 PM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote: > > > > > > > > I was debating between WARN-ing here, and returning -ENOMEM and > > > > WARN-ing at shmem's callsite. > > > > > > > > My thinking is that if we return -ENOMEM here, it will work in the > > > > current setup, for both shmem and other callsites. However, in the > > > > future, if we add another user of swap_duplicate_nr(), this time > > > > without guaranteeing that we won't need continuation, I think it won't > > > > work unless we have the fallback logic in place as well: > > > > > > > > while (!err && __swap_duplicate(entry, 1, nr) == -ENOMEM) > > > > err = add_swap_count_continuation(entry, GFP_ATOMIC); > > > > > > Sorry, I accidentally sent out the email without completing my explanation :) > > > > > > Anyway, the point being, with the current implementation, any new user > > > would immediately hit a WARN and the implementer will know to check. > > > > > > Whereas if we return -ENOMEM in __swap_duplicate(), then I think we > > > would just hang, no? We only try to add swap count continuation to the > > > first entry only, which is not sufficient to fix the problem. > > > > > > I can probably whip up the fallback logic here, but it would be dead, > > > untestable code (as it has no users, and I cannot even conceive one to > > > test it). And the swap abstraction might render all of this moot > > > anyway. > > > > What I had in mind is not returning -ENOMEM at all, but something like > > -EOPNOTSUPP. The swap_duplicate_nr() will just return the error to the > > caller. All callers of swap_duplicate() and swap_duplicate_nr() > > currently check the error except shmem. > > ..and just to be extra clear, I meant WARN _and_ return -EOPNOTSUPP. Ah ok this makes a lot of sense actually. I'll return -EOPNOTSUPP here. Do you think warn within __swap_duplicate() makes more sense, or at shmem's callsite make more sense? I feel like we should warn within __swap_duplicate callsite. That way if we accidentally screw up for other swap_duplicaters in the future, the feedback will be immediate :)
