linux-nfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dean Hildebrand <seattleplus@gmail.com>
To: Boaz Harrosh <bharrosh@panasas.com>
Cc: Fred Isaman <iisaman@netapp.com>, linux-nfs@vger.kernel.org
Subject: Re: [PATCH 00/22] LAYOUTGET invocation
Date: Wed, 26 May 2010 11:53:45 -0700	[thread overview]
Message-ID: <4BFD6E39.6010604@gmail.com> (raw)
In-Reply-To: <4BFD64B6.5070109@panasas.com>



Boaz Harrosh wrote:
> On 05/26/2010 08:39 PM, Dean Hildebrand wrote:
>   
>> Try to remember that this isn't some new feature that we are disabling, 
>> or a new way of doing things, this is a primary I/O path. We MUST fix 
>> this with the B-list code submission, so why go through the hassle of 
>> searching through old patches and tags to find it.
>>     
>
> No, this is no hassle. uncommenting old code and hopping for the best
> is the hassle. (insert here the explanation from previous mail)
>   
To answer Fred's statement, I understand the nfs o_direct will still 
work, but pNFS must support o_direct in the b-list.  O_direct is not a 
wierd unused flag, it is very common.  Also, I wouldn't be hoping for 
the best, I would actually fix it....
>   
>> If you want to talk about a *REAL* solution, then we need to figure out 
>> who broke O_DIRECT and reject their patches until they fix it.  You 
>> can't submit patches that break a primary I/O path.  But again, since we 
>> are focused on A-list items, ifdef'ing the code out for now in the 
>> B-list branch seems like a reasonable compromise.
>>
>>     
>
> No "ifdef'ing the code out" is never a "reasonable compromise" if you want
> then keep it in a separate clean patch, with "BROKEN:" at subject line and
> committed in a branch that is above the pnfs-all-latest. So it can be rebased
> from time to time, but not included in the regular test scenario, until fixed.
>
> (Which is BTW what it means the keep it out-of-tree, these git games are done
>  routinely, every day)
>   

My point is that someone's patches broke O_DIRECT, and this is ONLY 
acceptable because it is in the B-list.  So temporarily having code in 
the B-list that is #ifdef'd out really doesn't seem like the worst idea 
in the world.  But either way, as long as its isn't simiply removed (as 
the original patches would have done) and is easy to add back in so that 
we can figure out what went wrong and fix it up.
Dean
> Boaz
>
>   
>> Dean
>>
>> Boaz Harrosh wrote:
>>     
>>> On 05/25/2010 11:14 PM, Dean Hildebrand wrote:
>>>   
>>>       
>>>>     
>>>>         
>>>>> I can send some post_submit patches with the code ifdef'ed out if people would be content with that.
>>>>>   
>>>>>       
>>>>>           
>>>> Thanks for the background.  I would be much happier if you sent patches 
>>>> with the code ifdef'd out, added with the comment in the code regarding 
>>>> which patches you believe introduced the problem.
>>>>
>>>> Dean
>>>>     
>>>>         
>>>>> Fred
>>>>>       
>>>>>           
>>> I disagree. Source code is not a version management system. We have git
>>> for that. The code is never lost it is there for eternity in the git
>>> tree. We could ask Benny to tag the last branch that had broken directIO
>>> as LAST_directIO_VERSION for easy random access at future time.
>>>
>>> If in the future someone smart wants to forward port the code and fix it
>>> then the *right* way to do it is by manual octopus merge at the point of
>>> branch.
>>> Never, Never uncomment out code that was sitting collecting dust.
>>> Manual octopus merge I mean using the two diffs from the two sides of the
>>> branch, and replaying one on the other. For instance if at one patch
>>> a function was moved, then redo the move of the current function again, not
>>> leave the old code as it was before. Let the merge point out the points of
>>> friction. Because you see, with commented code, there is never a merge
>>> conflict it will always uncomment.
>>>
>>> And anyway the Kernel people will never accept code in comments. There
>>> are out-of-tree gits to do that. So I don't even think it is an option.
>>> The pnfs branches are patches that should eventually go upstream. Or
>>> are currently the only option for the testing of upstream code.
>>>
>>> Boaz
>>>   
>>>       
>
>   

      reply	other threads:[~2010-05-26 18:53 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-05-16  1:22 [PATCH 00/22] LAYOUTGET invocation Fred Isaman
2010-05-16  1:22 ` [PATCH 01/22] Revert "pnfs-nonfilelayout: Prelim support for non-file layout O_DIRECT" Fred Isaman
2010-05-16  1:22   ` [PATCH 02/22] Revert "pnfs: Enable O_DIRECT write path." Fred Isaman
2010-05-16  1:22     ` [PATCH 03/22] Revert "pnfs: Enable O_DIRECT read path." Fred Isaman
2010-05-16  1:22       ` [PATCH 04/22] Revert "pnfs: Add function to set up O_DIRECT I/O" Fred Isaman
2010-05-16  1:22         ` [PATCH 05/22] pnfs: filelayout: clean and breakup nfs4_pnfs_dserver_get Fred Isaman
2010-05-16  1:22           ` [PATCH 06/22] pnfs: filelayout: remove some dead code from filelayout_commit Fred Isaman
2010-05-16  1:22             ` [PATCH 07/22] pnfs: remove PNFS_LAYOUTGET_ON_OPEN Fred Isaman
2010-05-16  1:22               ` [PATCH 08/22] pnfs: track the number of outstanding commits Fred Isaman
2010-05-16  1:23                 ` [PATCH 09/22] pnfs_submit: mandate basic io path operations for layout drivers Fred Isaman
2010-05-16  1:23                   ` [PATCH 10/22] pnfs_submit: expose pnfs_update_layout, put_lseg, and get_lseg functions Fred Isaman
2010-05-16  1:23                     ` [PATCH 11/22] pnfs_submit: stash and refcount lseg in read path Fred Isaman
2010-05-16  1:23                       ` [PATCH 12/22] pnfs_submit: read path changeover Fred Isaman
2010-05-16  1:23                         ` [PATCH 13/22] pnfs_submit: use fsdata to pass lseg Fred Isaman
2010-05-16  1:23                           ` [PATCH 14/22] pnfs_submit: stash and refcount lseg in write path Fred Isaman
2010-05-16  1:23                             ` [PATCH 15/22] pnfs_submit: remove pnfs_file_operations Fred Isaman
2010-05-16  1:23                               ` [PATCH 16/22] pnfs_submit: remove pnfs_update_layout_commit Fred Isaman
2010-05-16  1:23                                 ` [PATCH 17/22] pnfs_submit: remove pnfs_writepages LAYOUTGET invocation Fred Isaman
2010-05-16  1:23                                   ` [PATCH 18/22] pnfs: export some commit error handling for use by layout drivers Fred Isaman
2010-05-16  1:23                                     ` [PATCH 19/22] pnfs_submit: API change: remove pnfs_commit layoutget invocation Fred Isaman
2010-05-16  1:23                                       ` [PATCH 20/22] pnfs_submit: filelayout: rewrite filelayout_commit to use new API Fred Isaman
2010-05-16  1:23                                         ` [PATCH 21/22] pnfs_submit: remove unecessary pnfs_fl_call_data field pnfs_client Fred Isaman
2010-05-16  1:23                                           ` [PATCH 22/22] pnfs_submit: remove unecessary pnfs_fl_call_data field commit_through_mds Fred Isaman
2010-05-25 18:27 ` [PATCH 00/22] LAYOUTGET invocation Dean Hildebrand
2010-05-25 19:03   ` Fred Isaman
2010-05-25 20:14     ` Dean Hildebrand
2010-05-26  8:43       ` Boaz Harrosh
2010-05-26 17:39         ` Dean Hildebrand
2010-05-26 17:58           ` Fred Isaman
2010-05-26 18:13           ` Boaz Harrosh
2010-05-26 18:53             ` Dean Hildebrand [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4BFD6E39.6010604@gmail.com \
    --to=seattleplus@gmail.com \
    --cc=bharrosh@panasas.com \
    --cc=iisaman@netapp.com \
    --cc=linux-nfs@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).