Hi Nicholas,
Thanks for reviewing this. Please see my comments below.
On Fri, Mar 20, 2015 at 08:00PM, Nicholas A. Bellinger wrote:
> Hi Ilias,
>
> Thanks for your patch. Comments are inline below.
<snip>
> > diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
> > index 2de6fb8..ca9fa61 100644
> > --- a/drivers/target/target_core_pr.c
> > +++ b/drivers/target/target_core_pr.c
> > @@ -78,6 +78,22 @@ enum preempt_type {
> > static void __core_scsi3_complete_pro_release(struct se_device *, struct se_node_acl *,
> > struct t10_pr_registration *, int, int);
> >
> > +static inline int is_reservation_holder(
>
> No need for an explicit inline. The compiler will do the right thing.
ACK.
<snip>
> > @@ -1849,7 +1866,7 @@ static int core_scsi3_update_aptpl_buf(
> > * Include special metadata if the pr_reg matches the
> > * reservation holder.
> > */
> > - if (dev->dev_pr_res_holder == pr_reg) {
> > + if (is_reservation_holder(pr_res_holder, pr_reg)) {
> > snprintf(tmp, 512, "PR_REG_START: %d"
> > "\ninitiator_fabric=%s\n"
> > "initiator_node=%s\n%s"
>
> So doing the extra ALL_REG check here is wrong..
>
> APTPL metadata must have one res_holder=1 registration in order to
> determine which *pr_reg to set for dev->dev_pr_res_holder when
> rebuilding PR state.
>
> This is because dev->dev_pr_res_holder is a single *pr_reg as per our
> previous discussions and not a list, and for active registrations when a
> ALL_REG reservation is set, we just pretend it's the reservation holder
> too but don't actually change dev->dev_pr_res_holder.
>
> That said, please drop this part.
You are right, I didn't notice that. Thanks for the detailed
explanation. ACK.
<snip>
> > @@ -3375,7 +3385,7 @@ after_iport_check:
> > * From spc4r17 section 5.7.8 Table 50 --
> > * Register behaviors for a REGISTER AND MOVE service action
> > */
> > - if (pr_res_holder != pr_reg) {
> > + if (!is_reservation_holder(pr_res_holder, pr_reg)) {
> > pr_warn("SPC-3 PR REGISTER_AND_MOVE: Calling I_T"
> > " Nexus is not reservation holder\n");
> > spin_unlock(&dev->dev_reservation_lock);
>
> For register_and_move code this check is redundant btw, because an
> ALL_REG check already exists immediately after this one. I'd prefer to
> keep the existing comment + check for this special case. Please drop
> this part too.
I don't believe this check is redundant because it changes the warning
message that the user sees. Without this patch, if one tries the
REGISTER AND MOVE service action while holding an ALL_REG reservation
(but is not the same one with dev->dev_pr_res_holder) then the user will
see the warning message "Nexus is not reservation holder" which is
wrong. With the above change, the user will see the correct warning
message "Unable to move reservation for type ALL_REG". Should I leave
this part as is?
Thanks,
Ilias
> Rest of the patch looks fine.
>
> Care to address these three comments, and respin a -v2..?
>
> --nab
Attachment:
signature.asc
Description: Digital signature
