On Wed, Jul 26, 2023 at 07:59:17PM -0700, Nicolin Chen wrote:
- if (new_ioas) {
rc = iopt_add_access(&new_ioas->iopt, access);if (rc) {iommufd_put_object(&new_ioas->obj);access->ioas = cur_ioas;return rc;}iommufd_ref_to_users(&new_ioas->obj);- }
- access->ioas = new_ioas;
- access->ioas_unpin = new_ioas; iopt_remove_access(&cur_ioas->iopt, access);
There was a bug in my earlier version, having the same flow by calling iopt_add_access() prior to iopt_remove_access(). But, doing that would override the access->iopt_access_list_id and it would then get unset by the following iopt_remove_access().
Ah, I was wondering about that order but didn't check it.
Maybe we just need to pass the ID into iopt_remove_access and keep the right version on the stack.
So, I came up with this version calling an iopt_remove_access() prior to iopt_add_access(), which requires an add-back the old ioas upon an failure at iopt_add_access(new_ioas).
That is also sort of reasonable if the refcounting is organized like this does.
I just realized that either my v8 or your version calls unmap() first at the entire cur_ioas. So, there seems to be no point in doing that fallback re-add routine since the cur_ioas isn't the same, which I don't feel quite right...
Perhaps we should pass the ID into iopt_add/remove_access like you said above. And then we attach the new_ioas, piror to the detach the cur_ioas?
I sent v9 having the iopt_remove_access trick, so we can do an iopt_remove_access only upon success. Let's continue there.
Thanks Nic