Kairui Song <ryncsn@xxxxxxxxx> writes: > Huang, Ying <ying.huang@xxxxxxxxx> 于2023年11月22日周三 08:38写道： >> >> Kairui Song <ryncsn@xxxxxxxxx> writes: >> >> > From: Kairui Song <kasong@xxxxxxxxxxx> >> > >> > Move get_swap_device into swapin_readahead, simplify the code >> > and prepare for follow up commits. >> >> No. Please don't do this. Please check the get/put_swap_device() usage >> rule in the comments of get_swap_device(). >> >> " >> * When we get a swap entry, if there aren't some other ways to >> * prevent swapoff, such as the folio in swap cache is locked, page >> * table lock is held, etc., the swap entry may become invalid because >> * of swapoff. Then, we need to enclose all swap related functions >> * with get_swap_device() and put_swap_device(), unless the swap >> * functions call get/put_swap_device() by themselves. >> " >> >> This is to simplify the reasoning about swapoff and swap entry. >> >> Why does it bother you? > > Hi Ying, > > This is trying to reduce LOC, avoid a trivial si read, and make error > checking logic easier to refactor in later commits. The race with swapoff isn't considered by many developers usually. So, we should use a simple rule as much as possible. For example, if you get a swap entry, use get/put_swap_device() to enclose all code that operate on the swap entry. This makes code easier to be maintained in the long run. Yes. Sometimes we break the rule a little, but only if we have enough benefit, such as improving performance in some practical use cases. > And besides there is one trivial change I forgot to include in this > commit, get_swap_device can be put after swap_cache_get_folio in > swapin_readahead, since swap_cache_get_folio doesn't need to hold the > swap device, so in cache hit case this get/put_swap_device() can be > saved. swapoff is rare operation, it's OK to delay it a little to make the code easier to be understood. > The comment also mentioned: > > "Then, we need to enclose all swap related functions with > get_swap_device() and put_swap_device(), unless the swap functions > call get/put_swap_device() by themselves" > > So I think it should be OK to do this. No. You should change the code with a good enough reason. Compared with complexity it introduced, the benefit isn't enough for me so far. -- Best Regards, Huang, Ying
