Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1654795 > unrolled thread

linux-next: manual merge of the target-bva tree with the target-updates tree

Back to article view | Back to linux.kernel

Contents

  linux-next: manual merge of the target-bva tree with the  target-updates tree Stephen Rothwell <sfr@canb.auug.org.au> - 2017-06-01 06:20 +0200
    Re: linux-next: manual merge of the target-bva tree with the  target-updates tree "Nicholas A. Bellinger" <nab@linux-iscsi.org> - 2017-06-01 06:30 +0200
      Re: linux-next: manual merge of the target-bva tree with the  target-updates tree Bart Van Assche <Bart.VanAssche@sandisk.com> - 2017-06-01 07:10 +0200
        Re: linux-next: manual merge of the target-bva tree with the  target-updates tree "Nicholas A. Bellinger" <nab@linux-iscsi.org> - 2017-06-01 07:20 +0200
    Re: linux-next: manual merge of the target-bva tree with the  target-updates tree Bart Van Assche <Bart.VanAssche@sandisk.com> - 2017-06-01 07:10 +0200
      Re: linux-next: manual merge of the target-bva tree with the  target-updates tree "Nicholas A. Bellinger" <nab@linux-iscsi.org> - 2017-06-01 07:10 +0200
        Re: linux-next: manual merge of the target-bva tree with the  target-updates tree Stephen Rothwell <sfr@canb.auug.org.au> - 2017-06-02 01:30 +0200
        Re: linux-next: manual merge of the target-bva tree with the  target-updates tree "Nicholas A. Bellinger" <nab@linux-iscsi.org> - 2017-06-02 06:00 +0200

#1654795 — linux-next: manual merge of the target-bva tree with the target-updates tree

Hi Bart,

Today's linux-next merge of the target-bva tree got a conflict in:

  drivers/target/target_core_transport.c

between commit:

  4ff83daa0200 ("target: Re-add check to reject control WRITEs with overflow data")

from the target-updates tree and commit:

  2c66660df665 ("target: Fix overflow/underflow handling of commands with a Data-Out buffer")

from the target-bva tree.

I fixed it up (I think (guidance appreciated), see below) and can
carry the fix as necessary. This is now fixed as far as linux-next is
concerned, but any non trivial conflicts should be mentioned to your
upstream maintainer when your tree is submitted for merging.  You may
also want to consider cooperating with the maintainer of the conflicting
tree to minimise any particularly complex conflicts.

-- 
Cheers,
Stephen Rothwell

diff --cc drivers/target/target_core_transport.c
index 6025935036c9,6cd49fe578a7..000000000000
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@@ -1164,37 -1164,7 +1164,21 @@@ target_cmd_size_check(struct se_cmd *cm
  			" %u does not match SCSI CDB Length: %u for SAM Opcode:"
  			" 0x%02x\n", cmd->se_tfo->get_fabric_name(),
  				cmd->data_length, size, cmd->t_task_cdb[0]);
 +
 +		if (cmd->data_direction == DMA_TO_DEVICE) {
- 			if (cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) {
- 				pr_err_ratelimited("Rejecting underflow/overflow"
- 						   " for WRITE data CDB\n");
- 				return TCM_INVALID_CDB_FIELD;
- 			}
 +			/*
 +			 * Some fabric drivers like iscsi-target still expect to
 +			 * always reject overflow writes.  Reject this case until
 +			 * full fabric driver level support for overflow writes
 +			 * is introduced tree-wide.
 +			 */
 +			if (size > cmd->data_length) {
 +				pr_err_ratelimited("Rejecting overflow for"
 +						   " WRITE control CDB\n");
 +				return TCM_INVALID_CDB_FIELD;
 +			}
 +		}
  		/*
- 		 * Reject READ_* or WRITE_* with overflow/underflow for
- 		 * type SCF_SCSI_DATA_CDB.
- 		 */
- 		if (dev->dev_attrib.block_size != 512)  {
- 			pr_err("Failing OVERFLOW/UNDERFLOW for LBA op"
- 				" CDB on non 512-byte sector setup subsystem"
- 				" plugin: %s\n", dev->transport->name);
- 			/* Returns CHECK_CONDITION + INVALID_CDB_FIELD */
- 			return TCM_INVALID_CDB_FIELD;
- 		}
- 		/*
  		 * For the overflow case keep the existing fabric provided
  		 * ->data_length.  Otherwise for the underflow case, reset
  		 * ->data_length to the smaller SCSI expected data transfer

[toc] | [next] | [standalone]

#1654798

On Thu, 2017-06-01 at 14:10 +1000, Stephen Rothwell wrote:
> Hi Bart,
> 
> Today's linux-next merge of the target-bva tree got a conflict in:
> 
>   drivers/target/target_core_transport.c
> 
> between commit:
> 
>   4ff83daa0200 ("target: Re-add check to reject control WRITEs with overflow data")
> 
> from the target-updates tree and commit:
> 
>   2c66660df665 ("target: Fix overflow/underflow handling of commands with a Data-Out buffer")
> 
> from the target-bva tree.
> 
> I fixed it up (I think (guidance appreciated), see below) and can
> carry the fix as necessary. This is now fixed as far as linux-next is
> concerned, but any non trivial conflicts should be mentioned to your
> upstream maintainer when your tree is submitted for merging.  You may
> also want to consider cooperating with the maintainer of the conflicting
> tree to minimise any particularly complex conflicts.
> 

Hi Bart,

The majority of this series hasn't been list reviewed, and shouldn't be
getting pushed in linux-next without some form of reviews.

I'll be getting back to v4.13 items now v4.12-rc fixes is out of the
way, but a weeks worth of list silence for your series doesn't mean
you're free to push un-reviewed stuff for drivers/target/ into
linux-next.

[toc] | [prev] | [next] | [standalone]

#1654802

On Wed, 2017-05-31 at 21:27 -0700, Nicholas A. Bellinger wrote:
> but a weeks worth of list silence for your series doesn't mean
> you're free to push un-reviewed stuff for drivers/target/ into
> linux-next.

I think this is an example of the pot calling the kettle black.
Your patch "target: Re-add check to reject control WRITEs with
overflow data" has not been reviewed by anyone but was pushed
into linux-next and sent to Linus anyway.

Bart.

[toc] | [prev] | [next] | [standalone]

#1654807

On Thu, 2017-06-01 at 05:05 +0000, Bart Van Assche wrote:
> On Wed, 2017-05-31 at 21:27 -0700, Nicholas A. Bellinger wrote:
> > but a weeks worth of list silence for your series doesn't mean
> > you're free to push un-reviewed stuff for drivers/target/ into
> > linux-next.
> 
> I think this is an example of the pot calling the kettle black.
> Your patch "target: Re-add check to reject control WRITEs with
> overflow data" has not been reviewed by anyone but was pushed
> into linux-next and sent to Linus anyway.

Heh, it fixed a regression you yourself pointed out.  :)

If your going to report a bug and not review the patch to address the
regression, I'm not going to let that regression slide to restore
existing behavior, just because you didn't bother to review the patch in
three plus weeks for the bug you reported.

Anyways, I'll get to your patches, but please get reviews on the list by
sending series that people want to review, instead of large unwieldy
series that intermix new features and random bug-fixes without any
context.

No wonder why people don't send time reviewing them!

[toc] | [prev] | [next] | [standalone]

#1654804

On Thu, 2017-06-01 at 14:10 +1000, Stephen Rothwell wrote:
> Hi Bart,
> 
> Today's linux-next merge of the target-bva tree got a conflict in:
> 
>   drivers/target/target_core_transport.c
> 
> between commit:
> 
>   4ff83daa0200 ("target: Re-add check to reject control WRITEs with overflow data")
> 
> from the target-updates tree and commit:
> 
>   2c66660df665 ("target: Fix overflow/underflow handling of commands with a Data-Out buffer")
> 
> from the target-bva tree.
> 
> I fixed it up (I think (guidance appreciated), see below) and can
> carry the fix as necessary. This is now fixed as far as linux-next is
> concerned, but any non trivial conflicts should be mentioned to your
> upstream maintainer when your tree is submitted for merging.  You may
> also want to consider cooperating with the maintainer of the conflicting
> tree to minimise any particularly complex conflicts.

Hello Stephen,

Thanks for having fixed this up. I hadn't noticed that Nic had queued up patches
that conflict with my patches. I will rebase my tree.

Bart.

[toc] | [prev] | [next] | [standalone]

#1654805

On Thu, 2017-06-01 at 04:59 +0000, Bart Van Assche wrote:
> On Thu, 2017-06-01 at 14:10 +1000, Stephen Rothwell wrote:
> > Hi Bart,
> > 
> > Today's linux-next merge of the target-bva tree got a conflict in:
> > 
> >   drivers/target/target_core_transport.c
> > 
> > between commit:
> > 
> >   4ff83daa0200 ("target: Re-add check to reject control WRITEs with overflow data")
> > 
> > from the target-updates tree and commit:
> > 
> >   2c66660df665 ("target: Fix overflow/underflow handling of commands with a Data-Out buffer")
> > 
> > from the target-bva tree.
> > 
> > I fixed it up (I think (guidance appreciated), see below) and can
> > carry the fix as necessary. This is now fixed as far as linux-next is
> > concerned, but any non trivial conflicts should be mentioned to your
> > upstream maintainer when your tree is submitted for merging.  You may
> > also want to consider cooperating with the maintainer of the conflicting
> > tree to minimise any particularly complex conflicts.
> 
> Hello Stephen,
> 
> Thanks for having fixed this up. I hadn't noticed that Nic had queued up patches
> that conflict with my patches. I will rebase my tree.
> 

Go ahead and get list review on drivers/target/ changes before pushing
them into linux-next, please.

Btw, I don't care if you queue up one's that do have at least two
Reviewed-bys into your tree, but everything that doesn't have
Reviewed-bys or Acked-by should not be going into linux-next.

[toc] | [prev] | [next] | [standalone]

#1655807

Hi Bart,

On Thu, 1 Jun 2017 14:14:06 -0700 Bart Van Assche <bart.vanassche@sandisk.com> wrote:
>
> On 05/31/17 22:04, Nicholas A. Bellinger wrote:
> > Go ahead and get list review on drivers/target/ changes before pushing
> > them into linux-next, please.
> > 
> > Btw, I don't care if you queue up one's that do have at least two
> > Reviewed-bys into your tree, but everything that doesn't have
> > Reviewed-bys or Acked-by should not be going into linux-next.  
> 
> It is not your job to rewrite the rules for linux-next. I'm following
> the guidelines I received from Stephen in December 2016. You were copied
> on the e-mail with guidelines Stephen sent to me. See also
> https://www.spinics.net/lists/linux-next/msg38488.html.
> 
> Stephen, if anything would have changed in the meantime that I'm not
> aware of please let me know.

This is what I tell everyone:

"You will need to ensure that the patches/commits in your tree/series have
been:
     * submitted under GPL v2 (or later) and include the Contributor's
        Signed-off-by,
     * posted to the relevant mailing list,
     * reviewed by you (or another maintainer of your subsystem tree),
     * successfully unit tested, and 
     * destined for the current or next Linux merge window.

Basically, this should be just what you would send to Linus (or ask him
to fetch).  It is allowed to be rebased if you deem it necessary."

Which is just what was in that message you pointed to.  Note the
"reviewed by you (or another maintainer of your subsystem tree)".  This
is more meant for the top level maintainers, but implies that the
patches have been reviewed, tested and are as ready as possible for
merging into the next level up tree.

-- 
Cheers,
Stephen Rothwell

[toc] | [prev] | [next] | [standalone]

#1655885

On Thu, 2017-06-01 at 14:14 -0700, Bart Van Assche wrote:
> On 05/31/17 22:04, Nicholas A. Bellinger wrote:
> > Go ahead and get list review on drivers/target/ changes before pushing
> > them into linux-next, please.
> > 
> > Btw, I don't care if you queue up one's that do have at least two
> > Reviewed-bys into your tree, but everything that doesn't have
> > Reviewed-bys or Acked-by should not be going into linux-next.
> 
> It is not your job to rewrite the rules for linux-next. I'm following
> the guidelines I received from Stephen in December 2016. You were copied
> on the e-mail with guidelines Stephen sent to me. See also
> https://www.spinics.net/lists/linux-next/msg38488.html.
> 
> Stephen, if anything would have changed in the meantime that I'm not
> aware of please let me know.
> 

The point is you're not sending PULL requests.

But like I said earlier, I really don't care if you put patches that
have been reviewed into your tree for linux-next before I get a chance
to review and pick them up for target-pending.

However, you putting random un-reviewed changes is where I have to draw
the line, especially considering what happened earlier in year where
what you had in linux-next close to the merge window was completely and
utterly broken.

Would you put un-reviewed block and scsi changes into linux-next..?

What would those subsystem maintainers say about that..?

Why is drivers/target any different..?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web